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
Hey Joe,
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)
Minor points:
- 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.
Best Answer
Great, you have a real opportunity to create value for your firm.
Your coworker should not be doing code review if she can't handle telling developers what's wrong with their code. It's your job to find problems and get them fixed before they affect customers.
Likewise, a developer who intimidates coworkers is asking to be fired. I've felt intimidated after a code-review - I told my boss, and it was handled. Also, I like my job, so I kept up the feedback, positive and negative. As a reviewer, that's on me, not anyone else.
Well, that's unfortunate, you say you're being tactful. You can find more to praise, if you have more to look for.
Critique the code, not the author
You give an example:
Avoid using the words "you" and "your", say, "the" changes instead.
Don't add rhetorical flourishes to your critiques. Don't make jokes, either. There's a rule I've heard, "If it makes you feel good to say, don't say it, it's no good."
Maybe you're buffing your own ego at someone else's expense. Keep it to just the facts.
Raise the bar by giving positive feedback
It raises the bar to praise your fellow developers when they meet higher standards. So that means the question,
is a good one, and worth addressing.
You can point out where the code meets ideals of higher level coding practices.
Look for them to follow best practices, and to keep raising the bar. After the easier ideals become expected of everyone, you'll want to stop praising these and look for even better coding practices for praise.
Language specific best practices
If the language supports documentation in code, namespaces, object-oriented or functional programming features, you can call those out and congratulate the author on using them where appropriate. These matters usually fall under style-guides:
Generic best practices
You could find points to praise on generic coding principles, under various paradigms. For example, do they have good unittests? Do the unittests cover most of the code?
Look for:
Functional Programming
If the language is functional, or supports the functional paradigm, look for these ideals:
Object Oriented Programming (OOP)
If the language supports OOP, you can praise the appropriate usage of these features:
under OOP, there are also SOLID principles (maybe some redundancy to OOP features):
Unix programming principles:
Unix principles are modularity, clarity, composition, separation, simplicity, parsimony, transparency, robustness, representation, least surprise, silence, repair, economy, generation, optimization, diversity, and extensibility.
In general, these principles can be applied under many paradigms.
Your criteria
These are far too trivial - I would feel condescended to if praised for this:
On the other hand, these are fairly high praise, considering what you seem to be dealing with, and I wouldn't hesitate to praise developers for doing this:
Writing down rules for passing code review?
That's a great idea in theory, however, while I wouldn't usually reject code for bad naming, I've seen naming so bad that I would reject the code with instructions to fix it. You need to be able to reject the code for any reason.
The only rule I can think of for rejecting code is there's nothing so egregious that I would keep it out of production. A really bad name is something that I would be willing to keep out of production - but you can't make that a rule.
Conclusion
You can praise best practices being followed under multiple paradigms, and probably under all of them, if the language supports them.