2011.09.18_code_reviews

What should I be looking for during Code Reviews?

For the longest time, I was wary of Code Reviews. I used to spend most of my time implementing features and felt that Code Reviews robbed me of my precious time. I was mistaken and have changed my approach.

Our team uses systematic Code Reviews to help us normalize our code base. It has helped us to set our expectations and to identify major defects before our clients got to experience them in production. We have a minimum of 2 reviewers for each code contribution and we encourage everyone to take part in Code Reviews. The practice has the added benefit of creating opportunities for developers to look at different parts of our solution.

Thus far, it’s been a great way to share knowledge and practices amongst our team members. I hardly hear anyone swearing during reviews and I rarely see any code that strays from our common expectations. It has made onboarding new team members easier because once they understand our standards, they’re able to contribute with code and Code Reviews.

Code Review tools don’t always provide enough context for each piece of code that we need to review. This makes it a challenge to perform effective Code Reviews in a timely manner. To make my life easier, I try to concentrate on things that don’t require me to understand the full context. For example, I look at standards, documentation, Unit Tests and try to use common sense to identify as many issues as I can.

The following questions help me perform quick and effective Code Reviews.

Why was the code changed?

Before we start reading any code, it’s crucial that we get acquainted with the associated Work Items. This will help us identify if the requirements were satisfied by the proposed solution. Furthermore, it will allows us to identify whether the developer has done more than what was expected.

Have Unit Tests been modified?

Modified Unit Tests are a great indicator that developers have tested their code. In projects where I use Test Driven Development (TDD), keeping an eye out for these changes becomes essential.

If the original task was to implement a new feature, then I should see new Unit Tests. On the other hand, if the requirement was to fix a bug, I should see a new Unit Test whose role is to prevent future regressions. The only time I don’t look for Unit Tests, is when the code was changed because of necessary refactoring efforts.

Has the internal documentation been updated?

At the beginning of my career, I had the opportunity to work with a very demanding employer. One of his requirements was that I needed complete documentation before I could start working on a new task. At first I found this process heavy and slow, but after a few months I understood that to go fast your have to go well. Eight months later, I came back and that documentation pay off! I had forgotten about the specifics of the system and would have looked like a fool without that documentation.

Today, I believe that developer documentation is essential and that it should be stored within the solution. This is especially useful when you need to onboard new developers or produce external documentation for partner, consumers and clients. Furthermore, Technical Writers can use this internal documentation as a base for their work.

Are Code Analysis Issue Suppressions accompanied by valid justifications?

When I start new projects I make the extra effort to resolve all the issues found by Code Analysis in Visual Studio. I try to leave as little suppressions as I can so that new developers who join the project are inclined to care about fixing these issues as they arise.

The Rule Set I use in my projects is a modified version of the “Microsoft All Rules” which contains the Extended Correctness Rules rule set for managed code, the Extended Design Guidelines Rules rule set for managed code, the Globalization Rules rule set for managed code, the Managed Recommended Rules rule set for managed code and the Security Rules rule set for managed code. This extensive Rule Set has helped me standardize code bases and follow proven industry standards.

When issues are identified in code that results from scaffolding or frameworks, developers can suppress specific warnings by adding attributes to classes and methods. For a suppression to be accepted, they must provide a valid justification so that other developers don’t have to ask questions like “Why isn’t this method static?”

Are TODO comments accompanied by a Work Item IDs?

Although I have nothing against TODO comments. I believe that we should track technical debt at its source. Consequently, I ask developers to create a Work Item in TFS for every TODO comment they add to our code base. As the project evolves, teams can use these Work Items to prioritized, free resources and tackle technical debt before it gets out of hand.

Are there any useless comments?

Some developers feel the urge to comment on obvious things. I prefer to think that comments are exceptional and should only be used when I fail to express my intent through code & good naming conventions. I strongly encourage developers to take this approach, because we rarely maintain comments and they contribute to misleading future developers.

Is there any dead code?

Dead code is just as bad as misleading comments. Don’t keep unused code in your solution because it’s just going to accumulate. Developers will be wary and won’t get rid of it because there’s too much uncertainty. What’s the worst that can happen? The solution will bloat, developers will get lost and productivity will drop off the map.

As professionals, we should strive to use source control for what its best at, keeping historic snapshots of our code. Keeping our code clean, means that we don’t have to ask unnecessary questions and that we can concentrate on real problems.

Can I tell which developer wrote the code?

Teams should strive to instate and respect coding standards. Writing code for a computer is the easy part. Writing code for your peers and for your future self, takes a bit of extra effort.

If I can tell which developer wrote a piece of code, I try to identify what makes it standout. Then I seek opportunities to adjust our coding standards based on my findings. If I can’t, I ask the developer to conform to our standards.

Are there any quality issues or possible bugs?

Once I’ve gone through process and standards, I look for possible quality issues and potential exceptions. This is a great opportunity to identity possible refactoring, technical debt and to question how business rules are applied.

Was the developer too creative?

The last question I ask myself about the code I’m reviewing, concerns creativity. Was the developer too creative with their proposed solution? Will other developers on our team understand and will they be capable of maintaining the code? If the answer to these questions is “NO”, I spend some time with the developer to simplify the overall solution.

Remember, simple is better!

12 responses to What should I be looking for during Code Reviews?

  1. 

    Hey Alex,

    I fully agree with all the points made in the post, at the same time I have a bit of an issue with this statement.

    “Code Review tools don’t always provide enough context for each piece of code that we need to review. This makes it a challenge to perform effective Code Reviews in a timely manner.”

    I feel that this kind of approach might be acceptable for a person who just started to contribute to a code base (although even that is debatable) but not for someone who knows it at least to some extent. Without understanding the context of the change and how it might impact the other code you might be missing the opportunity to catch serious logic or code dup problems.

    Liked by 1 person

    • 

      Agreed, at this time we use Stash which requires a developer to click to see more of the file.

      Seeing duplicate code spread over multiple files isn’t always easy to spot either. There are tools that help find this sort of code and I should write about hem in subsequent posts.

      Like

    • 

      Unit tests should help with finding logic errors and integration tests should help to get a better overview of the system. When I do code reviews I worry more about maintainability than execution because I use other tools to validate execution paths. So far, our stable and responsible components have 75% code coverage and we keep adding to it every day.

      Like

  2. 

    “Wary” not “weary”

    Like

  3. 

    Hi Alex,
    Thank you for this article. I agreed with all the points you mentioned. You might also want to write another article about how it is important to carefully prepare code review requests. Many code review requests that I see are prepared without keeping in mind how much efforts should be taken to provide a good quality of review. As you said at the beginning of the article “I was wary of Code Reviews”. I believe one of the reasons for that is a switching of a context that you are working in. From my point of view to help reviewers, the requesters, at least, should thing about next points:
    – Review request description should contain not only the copy-pasted summary of the bug / feature from issues trucking system (for ex. JIRA ) but also information about the reason of the bug, how it was fixed
    – Work done for code refactoring should not be mixed with other type of work (new features implementations, bug fixing, itc). Sometimes it becomes a mess of code modifications not related to a purpose of the work,
    – Before submitting a review request, it is worth that an author of the request does review himself.

    Regards,
    Mikhail

    Liked by 1 person

  4. 

    Hey Alex,

    Good write-up.

    I liked especially the unit-test-related portion.

    The tests are often an easy way to grasp what the change is about, as they lay it flat what`s being changed. Starting the review with these should help figuring out what a large code change is about…

    Thanks for sharing!

    Liked by 1 person

  5. 

    Just a few strokes on the pro side of Code Reviews (management and psycologicsl perspective):
    Goldrat’s http://en.wikipedia.org/wiki/Student_syndrome and http://en.wikipedia.org/wiki/Parkinson%27s_law (procrastination and buffer utilization) can be mitigated by this form of task control.
    As well in team work review from peers may instill more responsibility in individuals and mitigate the http://en.wikipedia.org/wiki/Ringelmann_effect

    Like

Trackbacks and Pingbacks:

  1. Dew Drop – May 5, 2014 (#1769) | Morning Dew - May 5, 2014

    […] What Should I be looking for during Code Reviews? (Alexandre Brisebois) […]

    Like

  2. What should I be looking for during Code Reviews? - Canadian Developer Connection - Site Home - MSDN Blogs - May 8, 2014

    […] Image Source: http://alexandrebrisebois.wordpress.com/2014/05/04/what-should-i-be-looking-for-during-code-reviews/ […]

    Like

  3. From my reading list #11 – May 12th, 2014 | Pascal Laurin - May 12, 2014

    […] What should I be looking for during Code Reviews? by Alexandre Brisebois […]

    Like

  4. How do you Build Features on #Azure? « Alexandre Brisebois - May 28, 2014

    […] code review process is all about verifying that we haven’t forgotten anything. We believe that internal […]

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s