Code Review Tips

Highlight the Positive

Not only ‘the bad’ and ‘the ugly’ should be pointed out. The good practices should also be highlighted. It is terribly frustrating for anyone who has code reviewed on a daily basis to only hear bad feedback about his/her work. A “+1” here in there is always welcome and might do miracles for motivating a fellow engineer.

Warnings

The warnings your IDE gives you are not just for show. Your IDE might be detecting bad practices, outdated tech stack and possible security risks. Do not ignore them.

Conventions & Code Style

Follow conventions and the code style from your team. Following the naming conventions for your variables, classes and packages helps in having code that is easy to read, to maintain and to search for. If everyone in a team works with the same rules, its output will be more consistent and reliable.

Documentation & Comments

I like to say that at least all public and protected methods should be documented. They should have a description of what they do and a description of their arguments and of what they return, if they do return anything. Documentation is very important for code maintainability in a team.

Comments might be useful here and there but prioritize writing simple code that is easy to read and include only relevant comments to keep the files concise and small.

Argument Validation

You can throw IllegalArgumentExceptions when the argument contract of your method is violated. The idea is to make sure your team members are using your method as it is intended to be used, while still in the development process, and not later, with the application already deployed to a client.

Test Code

There are several types of code tests, so I might make a blog post dedicated to the topic in the future. But the basic idea is that tests start becoming effective when there is enough coverage and that they should always be included for sensitive/core flows of the applications. In a good development process, any new feature should have tests and if there are none, it should be raised as part of the code review. For bugs, the missing test should be included or existing tests updated.

Input Sanitization

Input sanitization is a critical aspect of code review. It is important to ensure that user inputs are properly validated and cleaned to prevent security vulnerabilities such as SQL Injection and Cross-Site Scripting (XSS) and other types of injection attacks. The input from the client should never be trusted because it can be manipulated. Even though input sanitization is important for SQL Injection the first defensive measure should always be to use Prepared Statements. These are just the basics. A software engineer is not supposed to know everything about securing an application, that’s why it is also important to have dedicated security teams.

Conclusions

Being able to do constructive and useful code review requires experience. Not only programming experience but also experience in doing code reviews itself.

It is good if the tech lead has a document of best practices to share with the rest of the team. If it doesn’t exist, it doesn’t hurt to suggest it.

Minimize risks the best you can by following the best practices/rules. Keep the code clean and care for what you write and for its presentation. Also, always test your code and imagine edge cases, the developer should be the first QA. Have your critical thinking very active whenever you check a Stack Overflow/AI suggestion and don’t forget, defensive programming is king.

Recommendations

Since this is a blog dedicated to application security I suggest every developer to do Security Code Review 101 from owasp.org. It should only take a couple of minutes and it’s an important exercise for any development team.

If you find the website useful, please feel free to share it on social media and get in touch!