Hero image credit: Photo by Mikhail Nilov

When I’m doing a code review, I don’t view its purpose to be the same as a QA review. There is really a big difference in their purposes, with one minor exception that I’ll get in to in a moment. Code reviews are about code quality and the opportunity to share knowledge and learning across a team. Code reviews are NOT about catching bugs. If you spot a bug, great! But that’s not the purpose of a code review. Here’s a list of the things that I look for when I’m doing a code review.

Does It Address The Requirements?

First and foremost, does the code fulfill the requirements of the user story. This is the only part where a code review and a QA review overlap. As a first step of a code review, I review the user story and make sure I understand what the new/revised code is supposed to achieve. I can’t really determine if the code is good if I don’t understand what the code is supposed to accomplish.

I review the requirements, then walk through the code and determine if the code appears to successfully fulfill those requirements. Sometimes this is a quick and easy task. Other times, it is the longest part of a code review. If things are overly complex, then I need to understand why it was so complex. This serves as a good check on me, as an architect, and the business analysts I work with to determine if perhaps we should have broken a task out into more and simpler work items. That’s not always the case, but it’s a good point to review.

Does It Build & Run?

It may sound silly, but from time to time, I do hit a pull request where the code doesn’t build or run. It happens. We all make mistakes and sometimes we commit and push code that we’d swear we built and ran… but we didn’t.

Or, maybe it did build and run on their machine, but it doesn’t on mine. Don’t assume the problem is on them. Maybe I don’t have the correct runtime version installed? Maybe the code is .NET 8 and I only have v7 installed. Or maybe it’s Angular v14, and I only have v18 set up. It could be me. Never assume. Check first. And then reach out.

Style & Code Standards

Does the code meet your agreed upon style and coding standards. Are all variables and functions clearly and appropriately named. Are they using the correct casing? Are the braces in the correct places? And so forth. You do have defined code standards right?

Ok, ok. At a lot of places, there aren’t formal defined standards. I understand that. But there’s almost always a generally agreed upon way of organizing code and naming things. At the very least make sure they’re using descriptive names and not variable names like “x” or “a1”.

Note: There’s an eternal debate about whether or not to include the variable type as part of the name, things like “strSomeStringName” where the “str” indicates the variable is a string. I absolutely hate that “standard”. Places that require that are places I don’t want to work.

Another styling question is appropriate whitespace. Are they using approprate wrapping of code lines to make things more readable? Are they using tabs appropriately? (They are using tabs, and not spaces, right? That’s a whole other debate. But always pick tabs over spaces)

var students = await _context.Students
    .Where(x => x.DistrictId.Equals(id))
    .ToListAsync();

var game = new Game
{
    Name = name,
    Description = description,
    IsPublic = isPublic,
    TurnLimit = turnLimit,
    CurrentTurn = 0,
    MaxPlayers = maxPlayers,
    IsStarted = false,
    JoinCode = GenerateJoinCode(),
    CreatedDate = DateTime.Now,
    Id = Guid.NewGuid()
};

Unused Code

Next I look for any dead code. Are there unused references, dead code blocks, commented out code, and so forth. There should never be any code in there that isn’t actively being used. I should never see anything like the following:

/// No longer used
var x = 7;

/// Keep for later
async Task NoLongerUsedFunctionButMightNeedLater() {}

/// var lemon = new Fruit();

If it isn’t being used right now, get rid of it. You are using a code repo, right? If it needs to be restored later, you can pull it from the repo. Never keep dead code. Why? Because it never gets cleaned up later. It never gets used later. It just stays around and bloats your code. Get rid of it.

Redundant or Unneccesary Code

Closely related to unused code, but slightly different, are there pieces of code that are unneeded? For example, a .toString() on a variable that’s already a string, or enumerating a list multiple times, and so forth. Maybe they’re calling a function and then doing nothing with result. Or perhaps they’ve defined a function as async/await, but there’s no async code in the function. Those are the kinds of things that I’m looking for there.

Opportunities to Refactor & Simplify

Are there overly complex blocks of code? Or code that might be useful as its own function so it can be re-used by other code sections? These are great opportunities to refactor code so it stays as simple as possible. Simple code is readable code. And readable code is code that is easier to understand. You want your code to be as easily understandable as possible, because when you or someone else comes back years, or months, or even days later, you won’t remember why you wrote something a certain way or what it’s trying to do. Make your later life easier and make the code as simple as you can.

Do Changes To Existing Code Make Sense?

If the code has changed existing code, do those changes make sense? And did those code changes break any existing logic? You need to make sure that if code was changed, that change was appropriate and that those changes didn’t break anything. Well written unit tests can certainly help with this, but it’s also good to do that logical sanity check yourself to make sure.

Proper Exception Handling

Does the code appropriately check for and handle possible exceptions. Are they checking for things like communication failures to the database or external endpoints? Are they checking for valid values in variables or enumerables? For code languages that let you throw exceptions, is the correct exception type being used? For example, C# lets you throw the base Exception type, but you should generally never do so. You should always throw the most specific Exception type available, or create your own derived Exception type and throw that.

Appropriate Comments & Documentation

Is the code commented as appropriate. For example, do your functions and classes have header comments?

/// <summary>
/// This function sets up access to blob storage endpoints for the application to use
/// </summary>
/// <param name="app">The base application</param>
public static void AddBlobStorageEndpoints(this WebApplication app)
{

And for places where the code is more complex and not as self-explanatory as it could be, are comments added that clarify what the block of code is doing? Do the comments make sense and make the code more understandable?

Appropriate Tests

Does the code have appropriate unit and other tests set up? Do those tests all pass? Always build and run the tests as part of the review. Check those tests against the code. You don’t need 100% code coverage. That’s absurd and unneccesary and anyone that tells you that you need 100% doesn’t know what they’re talking about. But you do need appropriate coverage. Check that not only does the code have appropriate coverage, but are they also appropriately testing edge cases. It doesn’t need to test every edge case. But there should be an appropriate sample of edge cases covered so that you can get a general idea that some oddball situation isn’t likely to be a problem.

Teaching & Learning Opportunities

Most of all, code reviews are an opportunity to teach less experienced developers some of the things that I know. But it’s also an opportunity for me to learn new ways to do things that I hadn’t seen or thought of before. I don’t know everything. Neither do you. Seriously, you don’t. Take the opportunity to learn.

##S Conclusion

Code review and QA are not the same thing. QA is about meeting the user story goal and checking for bugs. Code reviews are about quality and learning. They’re different. They serve different purposes. Don’t consider them the same and don’t approach them in the same manner. Implemented correctly, both have great value.