5 Things to Look for During Code Review • Slash

Coding is a job that requires hours and hours of work with careful attention allotted to accuracy. However, even the most accurate coder is bound to make the rare mistake now and then, and this is what makes code review important for your development team. For one you can remove a bit of the stress caused by coding when a developer knows his/her code will be checked, but also it’s your chance to head off any potential issues before they even become issues. Let’s take a look at five key items to look for during code review.

Table of Contents

Convention

Convention, mutual agreement, it’s a must to gain within your development team. Everyone needs to be on the same page when coding so it’s imperative that the team follow the agreed upon coding rules and practices. For example, something as simple as using single vs double quotation marks, declaring short meaningful variable names, maintaining shorter lines of code per file and so on.

Consider this case: If we agree as a team to use camel case for a variable name, e.g. – waitedTime = 3000, but there is included code which is using snake case, e.g. – waited_time = 3000, now this may not break anything within the code, but it fails to follow the convention which the team has agreed to follow. Code is written by many developers and it’s necessary for it to merge together seamlessly as though it was written by one person. Following the team coding practices aids in development of really clean code that works as it’s designed to, so ensuring everyone’s code follows the agreed upon practices & rules is key.

Logic

Logic within the code is innately necessary as we gather you’re aware. So you may be thinking, “If it’s a given, why bother checking during code review?” Well, no one is perfect and as we’re all prone to mistakes, reviewing is a good way to catch those mistakes before they get released and become larger problems. Therefore, it’s good to review how people have written the code for specific logic/feature flow and complexity.

By doing this we can give suggestions to team members on best practices, and comment/feedback on how the logic should be rewritten according to the acceptance criteria of the review ticket or feature. For example: Consider a situation where there is a requirement to do a login API that verifies the password and generates & responds to the JSON Web token (JWT). In this case, we could review the full logic code as in what the team put in the JWT to encode. If perhaps they accidentally put the password inside the JWT, we can request them to revise that as naturally it will cause security vulnerabilities. So yes, logic is a given, but mistakes are bound to happen so it’s key to examine the logic during code review.

Complexity

The third thing which requires your attention during code review is complexity. In addition for the logic to make sense in the acceptance criteria, you must look out for code complexity in terms of the lines of code and conditional uses in the code. Those conditionals being: if – else, for loop, sub function or callback. Moreover, you must verify whether or not the code is testable.

Let’s consider the following example… A developer writes the login code in a controller under the MVC pattern, “login_controller.js”, using logic that contains many responsibilities. Those duties could be any number of things including:

API routing logic
Middleware logic
a definition of the user database model
a login code login comparing password
JWT logic to generate a token
response code logic with result and status (to the frontend)

Now as a result of this, we can see the code is too complex and we must make a corrective action. We could simplify it by splitting the responsibilities of those pieces of code into smaller files, import them to use in the “logic_controller.js” instead of having long lines of code. Those smaller files could be:

login_token.js: generates token function based on user data parameters
route.js: contain the routing logic of all APIs and include login route API in this file
guest_middleware.js: contain the middleware check if the user has not logged in for login route
user_model.js: contain the user database model code
response.js: contain the response code logic

Duplication

Fourthly you want to check for duplication. Reducing duplication wherever is possible should be an integral part of any code review. Again to better understand, let’s consider & add to the example from above:

response.js: can be reused in another part of the application, other controllers (in the MVC pattern on the Backend side) can reuse the response function by passing some parameters to it such as…

o status code: 200, 300 or 400

o status: true/false, body as the message response

guest_middleware.js: can also be reused for other API endpoints to check if the user has yet to login; e.g. – in the request reset password API

Now the target you’re aiming for here is to not have identical pieces of code and if by chance you do, then it’ll be necessary to write them in a way which enables you to reuse the same piece every time it’s needed. If this is not done, you will end up dealing with some unexpected/regression error.

Testability

Finally to cap things off, it’s time to verify for testability. All of the points above will aid greatly in making the code testable. However, we could also review additionally the way we write the unit testing to cover the code functionalities that have been written so far. Consider whether it’s best to test the functions or piece of code which we have extracted into smaller functions or files as per the steps above.

The reason for this is inherently obvious: we want to make sure the code is supposed to do what has been written. Therefore, any changes that come after it, must comply with the test cases, such as to add more cases, update the outcome of the test based on changes etc.

Let’s finish by wrapping up our example previously used. We’ve reviewed if we have the test cases covered for “login_controller.js” and if we have covered different cases of the “login_controller.js” and expect a different outcome that results in passing the different parameters to the “response.js” to respond accordingly. For example, if we simulate the test case of login failure, we review if we have passed the parameters correctly according to the login failure to the “response.js” or not. All in a day’s code review work!