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

Question

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

Personally, I'd push for the reviews. The fact is, that code isn't going to disappear over night, and I think we all know that //remove immediately really means //I'm never going to fix this, someone else has to.

Answer 2

Obviously the lack of code reviews (and other things) is causing tremendous technical debt in your project. It's going to come bite you in the ass some time soon.

Can you bring this problem up with your manager. I'm sure he/she will say they have to get the work done now, but bring it to his/her attention at least.

I don't think you are a code quality bitch. :)

Answer 3

I think each person is responsible for the code he writes, and the best (if not the only) way to ensure this, is through code reviews; so, thumbs up for Code Reviews

Answer 4

Clearly the code for this tool does need code reviewing. However, it is not your job to make this happen ... especially since someone else has been given formal responsibility for the tool.

My advice would be to keep reminding people that the code reviews need to happen, but don't get up peoples' noses about it.

Your main problem here seems to be a mix of developer culture, and under-resourcing. Really it is your management's job to fix these things.

Answer 5

This question clearly will be subjected to lots of opinions (and I am no exception!) You are absolutely right to insist on code reviews as a bare minimum standard. Much better than code reviews would be to get two people to write the code in the first place. Pair programming in my experience leads to a faster throughput time on feature implementations. Code reviews end up increasing your throughput time because you have to keep going back to the code, possibly days after it was written.

It seems that you have some "standard" code quality metrics and I would recommend using Checkstyle and Findbugs and have them run automatically so that a lot of your basic code reviewing is done by machines. With Checkstyle you can insist on maximum method lengths, method complexity, etc.

The 2nd paragraph will take longer to implement, but in the meantime, you are absolutely right to start with code reviews - I would be deeply afraid of code that has only seen one pair of eyes. And don't worry about offending anyone, if you can't take criticism on your code, you won't do well as a developer.

Answer 6

+1 for code reviews. I'd pushed for them in my previous job and was always told we didn't have time for it. My response was that it had the potential to save time by exposing flaws early...

Most of all, I thought it was a great learning/teaching tool. For one, we have junior developers who could use input from more experienced developers. And even experienced developers could learn something from others.

Answer 7

I don't think this is really a case of a Yes or No answer. Whilst I whole heartedly would recommend code reviews before committing to trunk, the political situation will determine the best course of action, and indeed the pressures and project type you are working on.

If you are working on a project that you must maintain either internally of for a client, the lack of good coding is going to hurt financially in the long term (even in the short term in project delays etc).

If the project is a quick delivery, and move on to the next project, the top management may be less inclined to agree with you.

My advice would be to talk to your manager. Explain your concerns and let him/her bring it up with the other team's manager. There is no lack of research to show why quality code will save money in the long term, so if you need to use this as justification (although I hope your manager is intelligent enough to understand the benefit of high quality coding) then do your research to back up you position. But DO stick to your principles, because they are good and sound. But DO NOT keep pushing a losing battle. If you have no chance of winning the battle, it will only upset your managers and colleagues and make you look bad, even though you are right!

And finally, good luck!