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!