Sunday, October 4, 2015

code reviews - yAy! or nAy!

"Regardless of what we discover, we understand and truly believe that everyone did the best job they could, given what they knew at the time, their skills and abilities, 
the resources available, and the situation at hand."
--Norm Kerth, Project Retrospectives: A Handbook for Team Reviews

Disclaimer: I will try to document my observations. Now, be warned, I may be entirely wrong and there is a fair possibility that it might not make ANY SENSE AT ALL.
But I am open to a discussion.

Having worked in a few Agile/Kanban/Hybrid/<insert_any_fancy_software_development_methodology_jargon> projects over the past few years, I have always had mixed feelings towards ‘Code Review’.

- As much as I have found code reviews a useful exercise, I have felt annoyed when a completed feature sat in code review status just because the reviewer didn’t have the time.

- Of all the encounters, this one grabs the cake. In one of my projects, the usual ritual was to include a distribution list with every pull request.
Now, usually, nobody would bother except the team which had a direct impact. But once in a while people would pop in (not knowing the context) and raise mindnumbingly insane questions often leading others to digress resulting in a ever growing mail chain heading nowhere.
That was a royal pain in the you-know-where. I can’t stress this enough.

- There were instances, where I was offended reading the comments. This one is an example.

 Man! I felt so ashamed that I wanted to wipe out the trail. I wanted this piece of check-in to be removed from the code (I swear to God. I did think about rewriting Git history.
But better sense prevailed. ) Slowly, it dawned to me that, the reviewer was critiquing the code and not the person who wrote it.
The reviewer would have said the same thing, had someone from his team had done it.

Reviewers won’t say a word if the author and the reviewer worked in same teams - MYTH BUSTED. No! It isn’t the case.

public void sendMessage(@QueryParam ("validate") @DefaultValue("true") String validationRequired, String jsonInput) {
// TODO: Why are we parsing the input argument as a json string?  This should be accepting the parsed object directly as a parameter.
// There are plenty of examples of this elsewhere in the in the code!

Code review is not:
- A bug hunt - No matter how hard we try and how many reviewers we had -- it doesn’t guarantee a defect free code. And how do we fix the gap? Add enough unit/integration/system tests to verify if things are working as expected.
- To drag the developer through the mud - we (the team is) are not here to spew hatred/frame opinions. The thing that gets reviewed is the code, and not the developer.
It’s solely up to the developer to learn from his/her mistakes.
- Is not a safety net - It doesn’t necessarily guarantee that you always have a few people to cover up for your mistakes. The liberty of one last check before it reaches production.
At the end of the day, you are responsible for the feature.

Code review is (the way I see it):
- A forum to discuss design decisions, raise questions and get them answered.
- A forum where you make yourself publicly accountable. You write a word the whole world notices. Ok! I am exaggerating. But you get the point right. Ensure that you have enough info
before you offer your opinion. When in doubt DONT. It’s ok to not know things once.  :)
- Compliment, reinforce, share good practices.
- Detect any slightest deviation which when unnoticed might change the way things are developed.

What do I look for when I review the code. (Note: Truth be told, I haven’t checked for these things meticulously on all occasions.)

1. Does the code meet the functional requirement?
2. Are there tests for the written code?
3. Is there an impact to the existing system?
4. Is the code consistent? Naming packages, methods, variable names etc
5. Does the code handle exceptions properly? Return codes, error messages
6. Can this be reused?
7. Is the logic straightforward?
8. Readability and maintenance. Logical flow, comments
9. Is the code optimized?
10. Is the code thread safe? (wherever applicable)

How developers can help
1. Ensure that the feature meets the functional requirement. If it’s a bug, ensure that it’s fixed.
2. Ensure that you add a brief description of the change. It needn’t be full blown. An abridged version of release notes would be perfect.
3. The testing strategy you’d followed. If it’s an endpoint, the URLs with the request/response would help.
4. Raise pull requests frequently so that we don’t have too much to review.
5. Annotate the code properly. Leave enough comments so that it helps others who read the code.


PS: I wrote this to educate the the junior developers in my team about code reviews. You might have heard the same thing in many different ways.  :) 

No comments:

Post a Comment