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.

@Path("/email")
@POST
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.

Thoughts?

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.  :) 

stand up, sit down - who cares

"We are AGILE. We aren't aversive to change. Instead, we welcome change with both hands."

"Look at this magic triangle. Pay attention to its edges and the ball within it. The moment you increase the size of the ball the triangle goes out of shape. Value, Cost, Quality are the edges and the scope is the ball."

"You don't do planning poker for estimation and you call yourself AGILE? What the BLEEPITY-BLEEP!"

Man! If I had a $ for every time I heard people say this, I would still be broke. :(

Stand-up meeting for starters is the one in which the team members gather around in circles and start telling each other about the past day's activities and what's in store for the day. Some lean back and start fiddling with their phones. That's a different story. ;-)

I've always looked at stand-up meetings as forums in which you(I) make yourself(myself) publicly accountable. At least to your team members, if not to a larger audience. It makes you push your limits to achieve something useful. Or, should I say tangible? Come on! Nobody likes to admit that they've been sitting on a issue for days together right? ;-)

If you look at the brighter side of things - You talk about issues, common problems, roadblocks and more often than not, a not-so-short explanation about how you had managed to solved a show stopper (KUDOS!) Eventually, you lose track of time. What was supposed to be a 15 minute meeting goes on and on for about 30-45 mins depending on the team size. It gets even worse with teams which are geographically separated.

"Mike, can you hear us? YES. NO. Now, it's better. We lost you again. I guess we are facing issues with the connection."
"We hear a lot of static. Can you please move away to a quieter place?"

Sky is the limit for the number of issues that we face during conference calls.

It is unfortunate and at times a little ironic - in a way that these theories carefully crafted to reduce the no. of meetings and mind numbing documentation ends up doing the same thing.

It makes me wonder. Are there any better alternatives?

What if we find a better way to collaborate? What if we don't do stand-ups and still call ourselves AGILE? Will that be a breach of any contract? As long as we manage to get the work done, who gives two hoots about how we call ourselves?

What's your take?

Disclaimer: I'm no saint. I do the same thing in stand-up meetings. At times, I tend to go overboard in an attempt to make everybody understand the concept so that we all are ON THE SAME PAGE.
I religiously follow this. I can't help it when I'm one of the participants. If I get a chance to host a meeting I make it a point to follow these rules.