How to tell someone that their mod's to my program are not good?

Question

G'day,

This is related to my question on star developers and to this question regarding telling someone that they're writing bad code but I'm looking at a situation that is more specific.

That is, how do I tell a "star" that their changes to a program that I'd written are poorly made and inconsistently implemented without just sounding like I'm annoyed at someone "playing with my stuff"?

The new functionality added was deliberatly left out of the original version of this shell script to keep the it as simple as possible until we got an idea of the errors we were going to see with the system under load.

Basically, I'd argued that to try and second guess all error situations was impossible and in fact could leave us heading down a completely wrong path after having done a lot of work.

After seeing what needed to be added, someone dived in and made the additions but unfortunately:

1. the logic is not consistent
2. the variable names no longer describe the data they contain
3. there are almost no comments at all
4. the way in which the variables are used is not easy to follow and massively decreases readability and hence maintainability.

I always try and approach coding from the Damien Conway point of view "Always code as if your system is going to be maintained by a psychopath who knows where you live." That is, I try to make it easy for follow and not as an advertisement for my own brilliance. "What does this piece of code do?" exercises are fun and are best left to obfuscation contests IMHO.

Any suggestions greatly received.

cheers,

Answer 1

If you have clearly defined coding standards for the project, point out that the code needs to be changed to meet those standards. The list you have there seems like quite reasonable feedback (though #3 is much argued-over; I would only push to document the really confusing parts as fixing the other three points, hopefully, makes the code less confusing).

Answer 2

If there are any other examples you have in your repository from this developer that are several months old, show one to him and ask him what it is doing. (Show him this one in a few months). When he has to zip around to find out what is actually in his variables, and deconstructing every line of code to figure out what it is doing. Break into a code review / pair programming session right there. Refactor and rename together so that he hopefully begins to see for himself exactly why these things are important.

Answer 3

I would just be honest about it. You don't necessarily need to point every little detail that's wrong, but it's worth having a couple of examples of any general points you're going to make. You might want to make notes about other examples that you don't call out in the first brief feedback, in case they challenge your reasoning.

Try to make sure that the feedback is entirely about the code rather than the person. For example:

Good: The argument validation in foo() seems inconsistent with that in bar(). In foo(), a NullPointerException is thrown if the caller passes in null, whereas bar() throws IllegalArgumentException.

Bad: Your argument validation is all over the place. You throw NullPointerException in foo() but IllegalArgumentException in bar(). Please try to be consistent.

Even with a "please," the second form is talking about the developer rather than the code.

Of course in many cases you don't need to worry about being so careful, but if you think they're going to be very sensitive about it, it's worth making the effort. (Read what you've written carefully, if it's written feedback: I accidentally included a "you" in the first version to start with :)

I've found that most developers (superstar or not) are pretty reasonable about accepting, "No, I didn't implement that feature because it has problem X." It's possible that I've been lucky though.

Answer 4

Coming from the other perspective, I would encourage you to think about it in their shoes. I will describe a "hypothetical" experience.

Some things to keep in mind:

• The guy was trying to do something good.
• Programmers are terrible at mind reading. They tend to only know what they read.
• He may have not been given complete guidance as what needs to be done(or what doesn't need to be done)
• He is likely doing the best he knows how to.

Just keep that in mind and talk to them. Teach them. No need for yelling or pissing contests. Just remember that they are not intentionally trying to make your life difficult.

Answer 5

I see that you've asked a lot of questions about how to deal with certain kinds of developers. It seems to be a common thread for you. You keep asking about how to change people around you. If this is a constant problem for you, then perhaps you are the problem.

Now I know you are asking questions to learn how to deal with people you find difficult, and that's good, however, you keep asking (and getting answers) about how to change people.

It seems to me that you need to change. Work with these people to change the code to what you want it to be. With them. Don't try to get them to do it. Just do it, and tell them what you did and why, and ask suggestions for further improvement, and learn from each other. Play off of each other's experience and strengths. Just my 2 cents.

Answer 6

Frankly, I think this is a political problem, not a coding problem. Specifically...

1. WHO SAID THIS PERSON WAS A "STAR"? If this is the same person you described in your other question, then you already have your answer there: THIS PERSON IS NO "STAR".

So then you get into the other effects of politics...

1. Who is claiming this person to be a star? Why can you not just tell the person "this is crap code"? Who is protecting them / defending them were you to do that? Can you do that or would you get blasted / demoted / put on the "to be laid off" pile?

You are asking questions that cannot really be answered in isolation. IF the code is crap, then throw it away and do it correctly yourself. IF there are reasons that you cannot do that, then you need to ask yourself if the benefits of this place outweigh the negatives.

Cheers,

-R

Answer 7

Creating a program and then releasing it to be worked on by other developers is tough. You are throwing your code to the mercy of others' development styles, coding conventions, etc.

Telling those developers that they are doing coding poorly, after the code is in, is one of the hardest things that you can do. It is best to address your concerns before they ever start working with your code. This can be done in two ways: Maintaining a detailed coding standard, requiring that submitted code adheres to this and maintaining a development road map, not to just outline when new features will be in, but to create dependencies to avoid such mishaps.

More to your situation, it is important not to criticize or it could cause hostility and worse code coming in. Maybe you can work with that developer to create standards documentation. You will be able to express your ideas about what the standards should be, and you will get their input, without causing any hard feelings.

Always point out the good things in their code, and be sure when discussing the weaknesses that you frame them pointing out the reasons that it will benefit everyone (the developer included), never criticize.

Good luck.

Answer 8

I would do the following:

• Make sure he knows that his hard work is appreciated (preferably, this should be the truth)
• Ask him if he would mind making a few changes, making it sound like no big deal and easy to fix
• Explain the issues, including why they are issues, and suggest specific changes to set him on the right path.

Hopefully, the exercise will help him integrate into the culture project better.

Answer 9

We try to solve these potential 'issues' proactively:

• Every 'bigger' project where people work together gets assigned a project 'codelead' (one of the developers). This rotates every project (based on preferences, experience with the particular task ...) so everyone gets to be in the 'contributing' and 'code-project-lead' roles once in a while.
• We explicitely made an agreement that these project 'leads' can decide whatever they want to with the code contributions of the others (sort of like a temporary dictatorship: change it, make suggestions, ask people to redo stuff etc.). The projectcode 'lead' bears the complete responsibility for the aggregated code to work.

With these formalised 'leads' (and the changing roles) I think people have less problems with (constructive) criticism of the parts they contribute.

Answer 10

Yes, keep the feedback as appreciative, professional and technical as possible, back up your concerns with possible "worst case" scenarios so that the disadvantages of those features and/or this particular implementation, become blatantly obvious.

Also, if this is about features/code that are very specific and are not of any use to most users, express your concerns about the code/use ratio - indicating concerns about increased code base complexity etc.

Ideally, present your concerns as open-ended questions - in the sense of: "Though, I am wondering if this way of doing it may work in the long term, due to ...". So that you actually encourage an active dialogue between contributors.

Invite your fellow contributors and user to provide their opinions on these concerns, in fact ask other people/contributors what they are thinking about this addition (in terms of pros & cons, requirements, code quality), do make the statement that you are willing to reconsider your current position if other contributors/users can provide corresponding insight.

You are basically encouraging an informal review that way, asking your community to also look into the proposed additions, so that the advantages and disadvantages can be discussed.

So, whatever the decision will be, it will be one that is community-backed, and not just simply made by you.

You being the architect of the original design, are also in an excellent position to provide architectural reasons why something is not (yet) suitable for inclusion/deployment.

If stability, complexity or code quality are a real concern, do illustrate how other contributions also had to go through a certain review process in order to be acceptable.

You can also mention how specific code doesn't really align with your current design, or how it may not scale too well with future extension to your current design, similarly you can highlight why certain stuff was left out explicitly.

If you actually like the features or the core idea, be sure to highlight the excellent addition these features would make if properly implemented and integrated, but do also highlight that the existing implementation isn't really appropriate due to a number of reasons.

If you can, do make specific suggestions for improvements, provide examples of how to do things better, and what to avoid and do express that you hope, this can be reworked to be added with the help of your project's community.

Ideally, present your requirements for actually accepting this contribution and do mention the background for your requirements, you may in fact say that you hate some of these requirements yourself.

Preferably, present and discuss instances where you yourself contributed similar code (or even worse code) and that you ended up facing huge issues due to your own code, so that these policies are now in place to prevent such issues. By actually talking about your own bad code, you can actually be very subjective.

Emphasize that you generally appreciate the effort itself, and that you are willing to provide the necessary help and pointers to bring the code in question into a better shape and form. Also, encourage that similar contributions in the future should be properly coordinated within your community, in order to avoid similar issues.

Always think in terms of features and functionality (and remind your contributor to do the same), not code - imagine it like a thorough code review process, where the final code that ends up being committed/accepted, may have hardly anything in common with the original implementation.

This is again a good possibility, to present examples where you yourself developed code that ended up largely reworked, so that much of it is now replaced by a much better implementation.

Similarly, there's always the issue with code that has no active maintainers, so you can just as easily suggest that you feel concerned about code that may end up being unmaintained, you could even ask if the corresponding developer would be willing to help maintain that code, possibly in a separate branch.

In the same sense, always require new code to be accompanied with proper comments, documentation and other updates. In other words, code that adds new or changes existing functionality, should always be accompanied with updates to all relevant documentation.

Ultimately, if you know right away that you cannot and will not accept any of that code in the near future, you can at least invite the developer to branch or even fork your project, possibly in you repository and with your help and guidance, so that you still express your gratitude for working with your project.