Code review affects software quality and reduces the risk of bugs. A well-executed code review leads to fewer bugs, better understanding of the code and improves knowledge transfer. People who frequently review code write better code than the average programmer. This is because on the one hand they see more, and on the other they see more code.
Here are some things to look out for during the review.
1. Safety check
Secure your services. Create them robustly, expect unexpected behaviour and create fallback scenarios for every error.
Don't log sensitive information. This includes credit card details, permissions, passwords etc.
Don't create exceptions that pass on sensitive data. This can expose them, and the person calling the code can misuse this data.
Parameterize SQL queries using a java
PreparedStatement. This will help you avoid SQL injection.
Sanitize your input data. Focus especially hard on data from untrusted sources and remember that web form data may contain malicious data.
Incorrect data sanitisation can lead to a number of security issues such as Cross-Site Scripting (XSS) and XML injection.
2. Checking the tests
Pay attention to the tests for limit values. Testing a range of values? Choose the smallest value, the largest value for the range, and a little larger than the maximum.
Create "classes" of values and select one from each class. Example:
For a range of 1..100 we will have 3 classes. One from that range, one for numbers less than 1, one for numbers greater than 100.
Do not test only the successful paths. Test also the ones that don't succeed, it's also a good idea to test paths with exceptions, where you will encounter undefined behavior.
3. Checking access to Java classes
Limit access. Expose as little as possible, make it possible to convert what is public to private and protected.
Component dependencies should be oriented in a direction consistent with stability.
Look for what changes - this is what should be extracted into separate objects that you can then inject.
Prioritise composition over inheritance.
4. Final and static
Constants should be placed in
final class. This keeps them in one place. They can then be used in classes and tests.
Add a private constructor for static classes. This ensures that a static class will not have an instance.
Classes without subclasses should be marked as
final. This way, you limit inheritance where you don't need it
5. Factories & Builders
Prefer factories over constructors, as they disable the logic of object creation. Object creation, in most cases, is unrelated to business logic and can be used multiple times. Without factories, it is necessary to copy code.
Have multiple arguments in a constructor? Use the
builder pattern. You need to add all the attributes in the call chain and end everything by building an instance.
6. Checking the usage of null
null as method output, use Optional instead.
Optional makes the code cleaner. Working with the Optional interface is much more pleasant than with
null is meaningful, and sometimes it means "nothing". When
null means something, use
Optional, and if
null actually means "nothing", use
What does it mean if
null is something? An empty shopping cart is already something, a missing account is also already something. These are examples where
null is "something" and has meaning.
Do you not expect the output to be empty? Are you dealing with unexpected or undefined behaviour? Then use
When you work with lists, don't return
null. Given what I wrote above, an empty list is already "something". Return an empty collection, not
Programmers worry that returning an empty list will degrade performance, which is not true. You can use a non-mutable list to counteract this. Use
7. Code checking with exceptions
For unexpected scenarios, use exceptions. This will make it easier to find out what happened after the fact.
Handle all legitimate exceptions, and when you see one ignored, correct it. You can even get Sonar to tell you about such code. The calling code should know why the exception happened, but be sure not to reveal sensitive information.
Log what caused the exception. Fields, inputs, parameters - log them when an exception occurs.
null is "something". Return
null or throw an exception in unexpected situations.
Use checked exceptions when they can be handled and runtime exceptions for coding errors.
Unexpected behavior should result in an unchecked exception, because you can't create a handler to prevent it. In this situation it is best to log what you can and throw an exception.
null pointer exceptions and do your part to avoid them. NPEs are quite costly. How much? Depends on the stack, but most of the cost is the
fillInStackTrace method call. Experimental data from StackOverflow suggests that they slow down execution by up to 10 times.
- Restrict access to the inside of classes
- Use final classes and prevent the creation of instances of static classes
- Use factories to create instances
- Do not ignore exceptions
- Remember the difference between checked and unchecked exceptions
- Avoid exceptions where possible
The original article can be found here.