This is some advice I gave my team regarding code reviews recently. There are certainly many ways of doing something that are wrong and we must avoid them. On the other hand, there is rarely only one way of doing something that is right. All too often we assume that there is one right answer and that all else is, if not wrong, undesirable. That is rarely the case. Better does not necessarily mean more right. It’s too easy to waste time arguing over the ultimate solution when we have a perfectly adequate one in front of us already. This is true in both programming and design.
When it comes to code reviews, we must clearly point out and reject those things that are wrong. Into this bucket I throw bugs, coding standards violations, and maintenance issues. If the code contains a bug–that is it will cause incorrect behavior, crash the app, leak memory, etc.–it must be removed. It is not acceptable to check in code with known bugs in it. If the code violates the coding standards, it may not be intrinsically wrong, but it is wrong in this context and should be fixed. Finally, if the code will cause a maintenance issue like it is obfuscated, doesn’t allow for proper indirection, etc., it too must be fixed. Beyond these three items, anything brought up in a code review should be considered advice but the author of the change has final say. He is free to reject any advice he desires.
This is true not just of code reviews but of coding in general. There are often many ways to accomplish a task. Too often we engineers conflate the way we would do something with the right way of doing something. This explains some of our ever-present desire to rewrite everything. This isn’t productive though. If someone writes some code and it accomplishes the task, it is usually mistaken to ask that it be rewritten differently. Again, if there is a substantial issue with the code, it should be fixed. However, if there isn’t. If it is just a “this could be even better if…” observation, the author should be under no compulsion to change things. A smart programmer will listen to the advice of his colleagues and act on it often. A foolish one will always think he is right.
The same principles apply to design. The more visible the item you are working on, the more advice you’ll get. It is important to have one cook for each soup. Designers (both graphical and architectural) should solicit advice from everyone, but only act on the parts they want to. It is easy to get caught in gridlock. Person A recommends doing things this way. Person B recommends doing them that way. Both have valid points. Which to choose? Try this approach: Is A wrong? Is B wrong? If the answer to both questions is no, then the only wrong choice is the non-choice. Pick one and get on with things. Don’t take forever to decide.
This advice applies as much to the person giving the suggestion as to the person receiving it. If you tell someone a “better” way of doing things, don’t expect that they’ll just agree. Even if your way is objectively better, if their way already solves the problem at hand, let the previous decision stand. Make your suggestion, but don’t push it. You’ve said your piece. Now let the person responsible for the decision make it. Getting worked up about someone else adopting your better solution will be unpleasant for both parties.