What to look for in a Code Review: Upsource Quick Wins
We’ve had two themes running through the articles on what to look for in a code review:
- Lots of what we’re tempted to look for can (and should) be automated
- Humans are good at checking things computers can not.
In this final post in the series, we’re going to look at the grey area in between – we’re going to look at how the features in Upsource can make your job as a human code reviewer easier.
As a developer, code reviews can sometimes be a frustrating process – you’re used to all the power of your IDE, yet when you open up a set of changes in your code review tool you can’t leverage any of that. The cognitive load of having to navigate through the code in an unfamiliar way and losing all the context your IDE provides is one of the things that makes developers less keen to perform code reviews.
Let’s look at some of the features that Upsource provides that overcome these issues.
It might seem like a trivial thing, but the ability to navigate through the code via the code is something we simply take for granted when we use an IDE like IntelliJ IDEA. Yet the simple Cmd + click feature to navigate to a given class, or to find the declaration of a field, is often missing when we open code in a tool that is not our IDE. Upsource lets you click on a class name, method, field or variable to navigate to the declaration.
While this is very useful, something I find myself doing a even more in my IDE is pressing Alt + F7 to find usages of a class, method or field. This is especially useful during code review, because if a method or class has been changed in some way, you as the reviewer want to see what the impact of this change is – which means locating all the places it’s used. You can see from the screenshot above that this is easily done in Upsource – clicking on a symbol gives you the option to highlight the usages in the file, or to find usages in the project.
Intuitive navigation is great for a reviewer as it lets you browse through the code in a way that’s natural for you, rather than having some arbitrary order imposed on you – it makes it easier to see the context of the changes under review.
But there’s another IDE feature that would be extremely useful during code review – inspections. If you’re already using IntelliJ IDEA, for example, you’re probably used to the IDE giving you pointers on where the code could be simpler, clearer, and generally a bit better. If your code review tool offered the same kind of advice, you could easily check that all new/updated code doesn’t introduce new obvious issues, and possibly even cleans up long-standing problems.
Upsource uses the IntelliJ IDEA inspections – we actually covered how to enable them for Upsource in the last post. There are rather a lot of inspections available in IntelliJ IDEA, so we’re just going to give a taste of what’s possible – we’re going to cover some of the default ones that you may find useful during code review.
Exception Handling Issues
Inspections can catch potential problems around how error conditions are handled. For example, empty catch blocks.
It’s difficult to think of a time when catching and ignoring an Exception is the right thing to do. A code reviewer should be suggesting:
- Catching the Exception and wrapping it in a more appropriate one, possibly a
RuntimeException, that can be handled at the right level.
- Logging the Exception (we also touched on appropriate logging in the last post).
- At the very least, documenting why this is OK. If there’s a comment in the catch block, it’s no longer flagged by the inspection.
“Empty ‘catch’ block” is enabled in the default set of inspections. This and other related inspections can be found in IntelliJ IDEA’s inspections settings under Java > Error Handling.
There are a number of inspections available for “probable bugs”. These inspections highlight things that the compiler allows, as they’re valid syntax, but are probably not what the author intended.
String.format()issues like the one above.
- Comparing Strings using == not
- Querying Collections before putting anything in them (or vice versa).
- Accessing Collections as if they have elements of a different type (sadly possible due to the way Java implemented generics on collections).
Not all of these problems are automatically bugs, but they do look suspicious. They’ll usually require you, the code reviewer, to point them out to the author and have a discussion about whether this code is intentional.
Inspections to highlight all the potential problems listed are already selected by default. To find more inspections in this category, look under Java > Probable Bugs in the inspections settings.
Code can be simplified
It’s easy as you evolve code to end up with statements and methods that are more complicated than they need to be – it just takes one more bit of boolean logic or an additional
if statement. As code reviewers, we’re in a fortunate position of being one step back from the coal-face of the code, so we can call out areas ripe for simplification. Fortunately, we don’t have to do this alone – Upsource shows us some of these things automatically.
- Using explicit
falsein a boolean expression (in the example above this is unnecessarily verbose).
- Boolean expressions that can be simplified, or re-phrased to be simpler to understand.
whileexpressions that always evaluate to the same value:
- As with the other examples above, you may simply want to flag them in the code review so the author can use IntelliJ IDEA’s inspections to apply the recommended fix.
- In some cases, like
ifstatements that can be simplified in
equals()methods, the simplified code is not always easier to read. If this is the case, you may want to suggest the code author suppresses the inspection for this code so it is no longer flagged.
- In other cases, the inspection might be pointing to a different smell. In the
ifstatement above, the inspection shows this code (which is in a private class) is always called with a particular set of values so this
ifstatement is redundant. It may be viable to remove the statement, but as this specific example is only used in test code it implies there’s a missing test to show what happens when the two objects are equal. The code reviewer should suggest the additional test, or at least have the author document why it wasn’t needed.
These types of inspections can be found in Java > Control flow issues and Java > Data flow issues.
Upsource highlights all unused code (classes, methods, fields, parameters, variables) in a grey colour, so you don’t even need to click or hover over the areas to figure out what’s wrong with it – grey should automatically be a smell to a code reviewer.
There are a number of reasons a code review might contain unused code:
- It’s an existing class/method/field/variable that has been unused for some time.
- It’s an existing class/method/field/variable that is now unused due to the changes introduced in the code review.
- It’s new / changed code that is not currently being called from anywhere.
As a reviewer, you can check which category the code falls into and suggest steps to take:
- Delete the unused code. In the case of 1) or 2) above, this should usually be safe at the field/variable level, or private classes and methods. At the class and method level, if these are public they might be used by code outside your project. If you have control over all the code that would call these and you know the code is genuinely unused, you can safely remove them. In case 3) above, it’s possible that some code is work-in-progress, or that the author changed direction during development and needs to clean up left over code – either way, flag the code and check if it can be deleted.
- Unused code could be a sign the author forgot to wire up some dependencies or call the new features from the appropriate place. If this is the case, the code author will need to fix the unused code by, well, using it.
- If the code is not safe to delete and is not ready to be used, then “unused code” is at the very least telling you that your test coverage is not sufficient. Methods and classes that are used by other systems, or will be used in the very near future, should have tests that show their expected behaviour. Granted, test coverage can hide genuinely unused code, but it’s better to have code that looks used because it’s tested than have code that is used that is not tested. As the reviewer, you need to flag the lack of tests. For code that existed before this code review, you might want to raise a task/story to create tests for the code rather than to slow down the current feature/bug being worked on with unrelated work. If the unused code is new code, then you can suggest suitable tests. New code that’s untested should not be let off lightly.
- If you and the code author decide not to address the unused code immediately by deleting it, using it or writing tests for it, then at least document somehow why this code is unused. If there’s a ticket/issue somewhere to address it later, refer to that.
Inspections are not infallible, hence why they’re useful pointers for reviewers but not a fully automated check with a yes/no answer. Code might be incorrectly flagged as unused if:
- It’s used via reflection
- It’s used magically by a framework or code generation
- You’re writing library code or APIs that are used by other systems
These types of inspections can be found in Java > Declaration redundancy, Java > Imports and Java > Probable bugs. Or you can search for the string “unused” in the IntelliJ IDEA inspection settings.
And to make it even easier…
The navigation and inspection features are all available in the Upsource application. While it would be great if the app could provide everything we as developers want, sometimes we just feel more comfortable in the IDE. So that’s why there’s also an Upsource plugin for IntelliJ IDEA and other JetBrains IDEs, so we can do the whole code review from within our IDE. There’s also a new Open in IDE feature in Upsource 2.5 which, well, lets you open a code review in your IDE.
While many checks can and should be automated, and while humans are required to think about bigger-picture issues like design and “but did it actually fix the problem?”, there’s also a grey area between the two. In this grey area, what we as code reviewers could benefit from is some guidance about code that looks dodgy but might be OK. It seems logical that a code review tool should provide this guidance. Not only this, but we should also expect our code review tool to allow us to navigate through the code as naturally as we would in our IDE.
Upsource aims to make code review not only as painless as possible, but also provide as much help as a tool can, freeing you up to worry about the things that humans are really good at.