Doing code reviews (or peer reviews) for your Java codebase? Here's our collection of tips to make you an even better reviewer.
Just based on the term code review , you may immediately think of "looking for bugs" when hearing the term. While this is on our list, we'd argue reading the code with a focus on understandability (and readability) may be even more valuable!
For example, let's imagine we encounter the following code snippet during our review:
String url = getUrl(); if (url.startsWith("https://")) < url = "https://" + url; > With a helper method in place, we can reduce this to
String url = StringUtils.ensureStartsWith(getUrl(), "https://"); While this may not seem like much, small improvements like this add up quickly. This means there is less code to maintain, and (hopefully) less bugs as you can rely on "battle-tested" parts of your code base.
Doing things consistently helps you and your team to find your way around different the areas of your codebase - even the ones that you've never worked on before.
There are quite a few aspects around consistency, and maybe of them can (and should) be checked by linters, not humans.
But even with tools in place, there are still things you can look out for in your code review.
This one is obvious. Of course, code reviews are a big opportunity to spot bugs early and before they land in production.
Let's consider this example:
public boolean isValidUrl(String url) < return url.startsWith("http://"); > As an attentive reviewer, you'll probably see that this method will return false for URLs that use HTTPS, which is probably not what we want.
So take your time, focus on the code, and think about edge cases or scenarios that may not be handled correctly the code yet.
Were any tests added (or adapted) with the code changes? If not, this may be a good opportunity to clarify with the implementor if they can add additional tests, for example using JUnit (unit tests) or Selenium (end-to-end tests).
Sure, this may mean that the feature / bugfix will not be merged right away. But this will prevent regression bugs, and everyone on your team will feel more confident with the stability of your software.
Let's consider this simple example:
for (File inputFile : listInputFiles()) < Parser parser = createParser(getOutputFolder()); parser.parseFile(inputFile); > Reading the code carefully, we can see that parser is re-created on every loop iteration, but it is not dependent on the loop at all (for the sake of this example, let's assume that parseFile() has no side effects we need to worry about).
This means that as a reviewer, you could suggest to move the createParser() call out of the loop.
Now that we know what to look for during a code review, what should we (probably) not put onto our review checklist?
This is probably the most important rule of code reviews:
Automate all checks that can be automated
This includes things like:
Nitpicking (sometimes abbreviated with NIT) means pointing out trivial or unimportant things during code reviews.
While there is no clear definition of what exactly is (not) nitpicking, let's just bring a little common sense. Code reviews are important, but they shouldn't slow you down unnecessarily.
Here's an hypothetical example:
// TODO: NIT: We can't merge this! The comment should be one line only! /** * Reads markdown files from the given folder. */ public static void readInputFiles(String folder) < // . > Please don't be like that ๐
Code reviews for your Java code can be a super power if everybody involved understands what (and what NOT) to look for.
Focus on consistency, performance, testing, and other topics that you (as a human) can judge best. Have automation in place where possible, and don't waste everyone's time with useless discussions about trivial things.
Need to review a complex merge request?Let's get you some help! Simply paste your URL below:
