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.
This is one of the things that disturbs me about some of the arguments I've been hearing in favour of various frameworks, especially from people under 30 or recent graduates: "It makes you do the right thing!" (usually said with breathless glee). I don't personally have any time for an architectural straightjacket that removes my freedom of choice and expression in the service of someone's semi-religious notions of The One Right Way.
ReplyDelete@Kevin, I wholeheartedly agree. I was recently involved in creating some coding standards. I made sure we only mandated things that would create bugs or maintenance issues. When doing research for it though, I noticed that a lot of people out there advocate very strict standards with little justification. Often what they advocate is just a fad. For instance, there is a move in some circles to stop using the _Variable designation for member variables and instead call them just Variable but call them as this.Variable. I don't see that lasting but for now, it's a requirement in some places.
ReplyDeleteWith frameworks I think there is a happy median. Something like Ruby on Rails can feel like a straighjacket but on the other end of the spectrum is (language, not framework) Perl that is so open ended it's almost impossible to read someone else's code.
I agree, there are more ways to solve a problem.But I also believe there are experianced persons that should be able to force a rewrite even if the code is already functionally running and meeting the coding guidlines. In my current task as developer on a maintanance department I see code with a Cyclomatric complexity above 50 ! To me every thing that is above 30 is to much of a risk to accept into a maintenance group.
ReplyDeleteSo yes give the developers some space but also make a list with non functional requirements and check these like they where functional.
> it's almost impossible to read someone else's [Perl] code.
ReplyDeletePerl is admittedly pathologically eclectic, but it is also idiomatic. Once you learn a few idioms, you can read a surprisingly large cross-section of different developers' Perl.
But yes, when Perl is bad, it is very, very, bad. It would be interesting to see a code standard for Perl.