Code Review--an Excerpt from VM Brasseur's New Book Forge Your Future with Open Source
Excerpt from Forge Your Future with Open Source by VM (Vicky) Brasseur, Copyright © 2018 The Pragmatic Programmers LLC. Reproduced with the permission of the publisher.
Even new programmers can provide a lot of value with their code reviews. You don't have to be a Rockstar Ninja 10x Unicorn Diva programmer with years and years of experience to have valuable insights. In fact, you don't even have to be a programmer at all. You just have to be knowledgable enough to spot patterns. While you won't be able to do a complete review without programming knowledge, you may still spot things that could use some work or clarification.
If you're not a Rockstar Ninja 10x Unicorn Diva programmer, not only is your code review feedback still valuable, but you can also learn a great deal in the process: Code layout, programming style, domain knowledge, best practices, neat little programming tricks you'd not have seen otherwise, and sometimes antipatterns (or "how not to do things"). So don't let the fact that you're unfamiliar with the code, the project, or the language hold you back from reviewing code contributions. Give it a go and see what there is to learn and discover.
"But," you may wail, "how is that even possible?! I don't know how to program very well! How could I ever do anything valuable on a code review?" Calm yourself, friend. You have a lot to offer here. Earlier I mentioned pattern-spotting, and that's a good place to start. If the contribution you're reviewing looks a lot more complicated than everything around it, you've just spotted a potential problem. Does the code use different indentations or variable naming than elsewhere in the file? That's another potential problem. Is the code contribution really long, when everything else in the file is much shorter? That could be a sign something is wrong. You don't have to be that Rockstar Ninja 10x Unicorn Diva programmer to spot these things; you only have to be familiar with programming and—most importantly—you only have to be looking at the code.
Do be careful as you start code review for a project with which you're not very familiar. Some projects would rather not receive reviews from people who aren't yet skilled in the code in question, as those reviews often can contain errors or inconsistencies with how the project typically operates. Inexperienced reviewers also can confuse inexperienced contributors, who might not know that the person providing feedback to them is not very familiar with the code or the project. Always check the CONTRIBUTING
file or ask a core contributor before you start reviewing code contributions, rather than risk stepping on toes or providing feedback when none is wanted.
What to Look for in a Code Review
If you do decide to review code contributions, what kind of things should you look for? The answer, as you probably expect, is "it depends on the project." That said, there are several things you can keep in mind regardless of the project, the code, or the programming language being used. While it may seem like these tips are only for people earlier in their programming career, nothing could be further from the truth. What follows are best practices for code reviews by people of any experience level. Whether you're a neophyte or a master, these tips can help you spot potential problems in any code review.
- Does the code even pass the build? Does the project use continuous integration/continuous deployment (CI/CD) or otherwise have its test suite run automatically? If the test suite doesn't pass after the code contribution… You're a smart person. I don't need to tell you that this is a big red flag that something may be wrong with that code. Politely ask the contributor to study the build/test errors and correct them before you continue to invest your time in reviewing the contribution.
- Is the code even readable? You don't have to be an expert in a programming language to tell whether the code is readable. Strange loops, short and vague variable and function names, inconsistent use of whitespace or brackets, large blocks of commented out code… Many things could make a piece of code difficult to read, but the end result is the same: unreadable code is unmaintainable code.
- Do one thing and do it well. It's best practice that each class, method, or function in a program do one thing and do it well. This reduces the complexity of any one piece of code, making it shorter and much easier to understand, maintain, and test. Be on the lookout for any piece of code that's overloaded and trying to do too many things. A good clue for this can be that the code has a complicated or long conditional statement.
- The DRY Principle: Don't Repeat Yourself. Is there any code that occurs more than once, even if it's doing similar but not entirely identical things? If so, it should be refactored out into a separate class, method, or function. Repeated code means changes have to be applied in multiple places, leading to a higher chance of error. Plus, refactoring it out can make it easier to test.
- How is the error handling? Are errors explicitly handled? Are they even handled at all? Do errors include descriptive messages or are they vague, "an error has occurred"-type things? Proper error handling doesn't only make debugging a lot easier; it improves the experience for everyone who uses the program.
- Is the code efficient? More advanced programmers will have an easier time of determining this for a new piece of code, but even new programmers can have a feeling for whether code appears unwieldy, or whether it looks like it's working harder than it should to accomplish what it does. If your instinct tells you that the code may not be efficient, it may be worth flagging for more explanation.
- How is the test coverage? Does the code come with any tests? Both unit and integration? If the code was covered by existing tests, were they updated to make sure they're still valid? If it's new code or there were no tests before, did the author add any? If there are tests, don't forget to review those as well as the rest of the code.
- Does the code actually do what it's supposed to? If the code is intended to add a feature or fix a bug, compare the code against the issue it's supposed to close to make sure the code does what's expected of it. It's very easy to misunderstand expected functionality, forget to include a piece, or include more than is expected (more is not always better).
- Is the code documented? Code comments, installation instructions, user docs, API docs, troubleshooting docs… There are so many different ways a piece of code could or should be documented. Because documentation is so difficult, yet so important, it's usually easier to do it piecemeal as each new feature or bug fix is added to the repository. If the code you're reviewing doesn't come with changes to the documentation, you may want to suggest the author add some to help avoid the technical and usability debt that can accrue by skipping documentation.
As you can see, while knowing about code is very helpful when doing code review, there are a lot of things you can see and provide feedback on even if you're just getting started with programming. If the project is supportive of it, even less experienced programmers can provide a lot of valuable insights while also learning more about the project code and how it all fits together.