Feb 10, 2021
Peer Code Review: Best Practices
Improve development team collaboration
To review someone else’s code will be more than half of your job as a software engineer. Yet this topic is not well covered with good resources to know how to do a good code review.
🤔 The Why
There are many arguments on why you and your team should be doing code reviews but let’s check the important ones:
Detecting potential bugs
Each developer has a certain perspective to check the code. The more reviewers check it, the lower the risk to discover a potential security bug on production by chance.
Improve developers coding skills
The ideal company would have only senior developers. But since these companies don’t exist, doing code reviews will upskill all the developers including picking-up new coding techniques.
Onboarding new developers
Just letting new joiners check out the codebase will help them to understand how it works faster, what is the current coding style and how the new pull request is going to affect the repository.
Knowledge sharing
If each developer only writes their code and push it without no one seeing this code and understanding abstractly how it works. Then when the time comes for this developer to leave, it will be stressful for both the team and the developer.
They have to remember EVERYTHING and their substitute has to understand all of it which would be very challenging to almost impossible. This could potentially halt your product or slow it down dramatically.
📝 Checklist
During my research on doing code review the proper way, I have found this book “What to Look for in a Code Review” which had many valuable best practices to improve the process.
You can download the book for free from here:
https://leanpub.com/whattolookforinacodereview
Any code review should cover at least the following aspects to deliver a reasonably quality code: Tests, Performance, Data Structures, Security and Tooling.
🧪 Tests
Added Tests
Any new code you add or amend should have tests to cover these changes. If you fixed a bug, add a test to detect if it happens again. If you add a new feature, add tests to cover its requirements and expected behavior.
Understandable Tests
They should be like ready a story. It should be written well and clean like your typical codebase with no discrimination. Your future self or coworkers will probably have to read these tests to do any codebase changes.
Match the requirements
For each task, there are expected behaviors. The tests should always match and confirm that these behaviors are valid.
Document code limitation
If there are certain environment constraints, the tests should act as documentation for them and check if an exception is thrown.
Cover failure scenarios
Adding to the above point; when an exception is thrown due to a third-party service, the tests should cover how it is handled. Expect a service might return a 4xx or 5xx errors at any point for any reason.
Performance Tests
Slow system response is what kills new applications and you should always test if your application (both frontend and backend) are responding with acceptable time and bandwidth constraints.
🏃♂️ Performance
Performance requirements
You should check if there are performance requirements for the added code. Will the new code take a long time to respond? will it slow down existing services? will it increase the cloud service billing costs?
Third-party services
It’s essential to check if the code will do expensive calls to the third-party services. Like if it is calling the database, is it calling it multiple times while batching in a single SQL command is possible.
Race conditions
Root for many evils in the software industry. Any new code should be reviewed and checked if it is in any way that would cause a race condition. Like when the API call depends on the user token while it might not exist yet.
Memory Leaks
It can deteriorate both the frontend and backend. The first one will freeze the browser and the second one will make your company go broke. It is famous when you use recursion. Make sure the new code can catch and avoid memory leaks.
Make sure things like removeEventListner or unsubscribe are always added to protect your precious application.
Closing connections and streams
If the new code opens a connection to steams like databases, ports, and files, they must always close them to release these resources for new requests. Also to avoid possible database timeout or potential crash.
Caching
Check if the new code caches third-party requests when possible. You will improve the time constraints of the application and the number of maximum requests that could be handled.
🏗 Data structure
Correct data structure
This will affect the code performance and memory usage. Like for example, arrays should be used for sorted things while hashmaps or dictionaries should be used for quick search of unordered items.
During college, we had an assignment to process a huge amount of data and my code was able to do it all under 2 minutes while others would take over 30 minutes just because I decided to use a 4-D matrix instead of 3-D matrix data structure.
Simplicity
Avoid using a complex data structure, this will make it hard for new joiners and your future self. This one would be easy to catch during a code review since you will probably question how it works from the code owner.
SOLID Principles
Any new code should follow all five SOLID principles. When reviewing code, detect any violation of them and ask the code owner for reasoning and most probably how to fix it.
- Single Responsibility Principle
- Open-Closed Principle
- Liskov Substitution Principle
- Interface Segregation Principle
- Dependency Inversion Principle
🛡 Security
MITM, CSRF, XSS…
These are a few of the most famous security vulnerablies that you should be aware of while reviewing the code. If you have no idea about them, you could check them out on YouTube or this quick tutorial
Used dependencies
Check if the used dependencies are updated and avoid potential bugs or security concerns. You can also automate this using Dependabot.
Access control
User authentication and authorization should be taken into consideration in the new code. Make sure the code uses the right token and permission to avoid potential data leakages that could potentially cost millions in fines.
Encryption
Always use encryption for transferring any sort of data, no exception. Tokens and passwords should be hashed. Always make sure HTTPS is used in the infrastructure as no justifiable reason to ever use HTTP today.
Application token secrets
If the pull request contains token secrets, they should be immediately removed from it and rotated at once.
Logging
This should be a topic on its own but in general, any action that happens due to code usage should be logged. If a resource is added, read, edited, or deleted all should be logged. With the intend that in case of bug investigation, it should be traceable.
⭐️ Quality
Codebase navigation
The main purpose is to keep everything organized and easily accessible using your editor or IDE quick search. Make sure committed code and files are well-placed in the correct place
Code inspection
There are automated tools for each language that allows you to inspect the code for potential code bugs and errors. Use automated tools to do that for every new pull request.
Unused code
Any code that is commented out or unused should not be inside the pull request and be removed. There are automated tools for this as well as plugins for your code editor.
Wrapping up
Code review might be a little bit exhausting but if it so then you are probably reading a bad code. It is essential for keeping the contributions of the whole team on track and have a maintainable and adaptable codebase.
