After some serious quality problems in the last year, my company has recently introduced code reviews. The code review process was quickly introduced, without guidelines or any kind of checklist.
Another developer and I where chosen to review all changes made to the systems, before they are merged into the trunk.
We were also chosen as "Technical Lead". This means we are responsible for code quality, but we don't have any authority to implement changes in the process, reassign developers, or hold back projects.
Technically we can deny the merge, giving it back to development. In reality this ends almost always with our boss demanding that it be shipped on time.
Our manager is an MBA who is mostly concerned with creating a schedule of upcoming projects. While he is trying, he has almost no idea what our software does from a business point of view, and is struggling to understand even the most basic customer demands without explanation from a developer.
Currently development is done in development branches in SVN, after the developer thinks he is ready, he reassigns the ticket in our ticketing system to our manager. The manager then assigns it to us.
The code reviews have lead to some tensions within our team. Especially some of the older members question the changes (I.e. "We always did it like this" or "Why should the method have a sensible name, I know what it does?").
After the first few weeks my colleague started to let things slide, to not cause trouble with the co-workers (she told me herself, that after a bug report was filed by a customer, that she knew of the bug, but feared that the developer would be mad at her for pointing it out).
I, on the other hand, am now known for being an ass for pointing out problems with the committed code.
I don't think that my standards are too high.
My checklist at the moment is:
- The code will compile.
- There is at least one way the code will work.
- The code will work with most normal cases.
- The code will work with most edge cases.
- The code will throw reasonable exception if inserted data is not valid.
But I fully accept the responsibility of the way I give feedback. I'm already giving actionable points explaining why something should be changed, sometimes even just asking why something was implemented in a specific way. When I think it is bad, I point out that I would have developed it in another way.
What I'm lacking is the ability to find something to point out as "good". I read that one should try to sandwich bad news in good news.
But I'm having a hard time to find something that is good. "Hey this time you actually committed everything you did" is more condescending than nice or helpful.
Example Code Review
I have some questions about your changes in the Library\ACME\ExtractOrderMail Class.
I didn't understand why you marked "TempFilesToDelete" as static? At the moment a second call to "GetMails" would throw an exception, because you add Files to it but never remove them, after you deleted them. I know that the the function is just called once per run, but in the future this might change. Could you just make it an instance variable, then we could have multiple objects in parallel.
... (Some other points that don't work)
- Why does "GetErrorMailBody" take an Exception as Parameter? Did I miss something? You are not throwing the exception, you just pass it along and call "ToString". Why is that?
- SaveAndSend Isn't a good name for the Method. This Method sends error mails if the processing of a mail went wrong. Could you rename it to "SendErrorMail" or something similar?
- Please don't just comment old code, delete it outright. We still have it in subversion.