Should I insist that we perform code reviews before merging back to trunk?

Question

Requested re-post from StackOverflow:

I'm working in a small development time with very limited time for development. We develop a tool that is important for the result of our work, but not used daily. I am the only person in the team that has a background as a programmer.

My problem is that I have been pushing for code reviews before merging back to trunk for over a year. Everyone agreed on this, but still it's only my code that has been reviewed. Returning from a long vacation I come back to a trunk with code comments as "this is an ugly solution - remove as soon as possible" and "quick fix". What also is new is that a guy has been appointed the responsibility for the tool. (A role that first was offered to me but I turned down due to a non work related reason.) And he thinks that this is an ok way to work: As we have such limited time to develop, we should cut corners like that.

My concern is that the other developers write ugly code: often breaking encapsulation, writing huge classes, adding internal classes at strange places, having few or no unit tests, and so on. It will eventually be impossible to develop the tool further.

Should I insist that we perform code reviews before merging back to trunk or am I just a code quality bitch?

Answer 1

i've been in similar situations before and imho it depends on "do I have to maintain the code".

If I have to maintain the code, than I want high quality code, I personally don't require code reviews for every commit (i.e. programmers can decide for themselves if certain code needs a review or not) but if the readability/maintainability suffers than it might be in order.

When reading this:

My concern is that the other developers write ugly code: often breaking encapsulation, writing huge classes, adding internal classes at strange places, having few or no unit tests, and so on. It will eventually be impossible to develop the tool further.

I do think that your problem is larger than just code reviews. You seem to be missing a few guidelines and/or they are not implemented. Having no/few unit tests might be a bad idea but depends on the specific case. However, breaking encapsulation, writing huge classes, ... really make bug-prone code so that should definately be fixed.

Answer 2

I think that code reviews and maintaining some coding guidelines is a good idea, but I think that doing it for each and every check in is a waste of time. It's a good idea when establishing a team and with young programmers, but experienced programmers can think for themselves and eventually you will have to trust them. That said - you can do the periodical code reviews to fresh thing up, but looking at each and every line of code that enters your VCS is really overdoing it.

And a small comments regarding your colleague's fixes - some times making an ugly fix is the correct solution. It may be that this particular code is not important enough to invest lots of time in, it may be that the simple solution is good enough and it's better to invest time in other things. Making your code "pretty" is not your main goal as a programmer. Your main goal is to deliver and obsessing on every line of code simply won't get you there.

What I'm trying to say is - you need to choose your battles. It's OK to lose the battle over some insignificant utility class to win the delivery war (or this REALLY IMPORTANT subsystem war, for that matter).

Answer 3

If the program in question is not a one-off throwaway prototype, I think code reviews should be mandatory for every check-in.

Senior developers could have a privilege of non-reviewed check-ins, once they are known to conscientious enough to request a review when appropriate.

Answer 4

Not sure code review is the answer until someone starts enforcing better coding standards. Someone writes crappy code, comments/admits it and checks it in anyway. What good is a review going to do if someone wants to reject the code, but it will put you behind schedule?

You're going to need to: set standards, supervise the major culprits more closely, and pound into their heads that good code doesn't always take more time write. They need to stop using timelines as an excuse for refusing to change bad habits.