The importance of code reviews

To become happy about your product code

Photo by Anthony DELANOIX on Unsplash

When you start building a product as a team, code review would be a benchmark of how well your team is organized.

A good team would make great use of code reviews and create their product rapidly with nice-looking code. Whereas a bad team would think of code reviews as just boring and meaningless tasks or even wouldn’t do any code review.

If so, why is it useful? How do we make use of it?

I’ll let you know my answer from my 3-month real experiences. Here I focus on Ruby on Rails app.

What are code reviews for?

1. Double checking of buggy code
2. Improve the readability
3. Integrate knowledge among team members
4. Standardize coding rule

And here is an important thing: everyone should be the one to review regardless of your skill level. Code reviews are good for Junior developers in that:

• They can learn from the code that senior developers write
• Junior developers tend to check more carefully
• It avoids senior developers from becoming lazy
• It gives the team psychologically safety, by removing the border between ‘who’s able to review’ and ‘who’s not’

Workflow of code review

1. Take much time to discuss the purpose of developing
2. Share basic structure (DDL, DML, model, route, controller) with your baddy
3. Start coding at your branch
4. Test your code by yourself
5. Make a pull-request and give a brief explanation
6. [reviewer] code review and give feedbacks
7. Modify the code and test
8. Done? Grab beers together! 🎉🎉

If you are using Github as a remote, make a pull request either from Github website or via command line.

For people with other cloud (or on-premise) servers as a remote, the following Git command will help reviewers do code review. Also, some source management tools have a built-in pull-request feature.

# let the branch name be 'bookmark'
$ cd [your project directory]
# create temporary directory without affecting your current work
$ git worktree add ../bookmark -b bookmark
$ cd ../bookmark
# pull the source code to review
$ git pull origin master
$ git pull origin bookmark
[... code review ...]
[You may need to adjust database configurations to check functions]
# delete temporary branch
$ git worktree remove --force ../bookmark
# for older version of Git, manually delete temporary folder and
$ git worktree prune

You may need to adjust database configurations, especially when DB schema is changed but no seed file is given, in order to actually check if it functions as expected. In Rails, config/database.yml declares database configurations.

config/database.yml

Things to consider (for reviewers)

Logic review

• Give boundary inputs or conditions ( 0 , nil , undefined, empty array:[], magic numbers such as pagination limit, etc) to variables
• Does it actually return something? (It doesn’t really matter for Ruby or Python, but really does for JavaScript)
• Check if it works when ActiveRecord methods or query returns nothing (DoesBook.where(author_id: params[:id]).first.title work?)
• Is it really OK to rescue there?
• Consider appropriate error handling (Should we send email, redirect, display 500, or ignore?)
• …usually it’s important to think from users’ point of view. “Who caused that error and what users expect/hope to happen then”
• Should a database transaction be set?
• …if yes, intentionally raise an error inside that transaction and confirm if it works
• Run fail-test code and see it actually fails
• Try SQL injections, manipulated HTTP requests and make sure they fail
• Try to open unauthorized url (i.e. GET /users/123 , GET /users/124,…)and see if it fails

Performance review

• Measure time to load the page and compare it with that of when you directly execute the same query from your database client
• Does it avoid N+1 problem?
• Tail log/development.log file to detect unnecessary queries (i.e. be careful using .count method on ActiveRecord)
• Check INDEX is labeled at frequently used column
• Check if there is unnecessary INDEX in the data table
• Consider image optimization

Readability review

• Avoid nested if statements as much as possible
• Use ternary operators or one-liner conditional statement
• Avoid condition-wrapped method (use return if ... )

• Don’t let constants be hard-coded
• …define them either global constants or class constants
• Avoid giving a vague name to a variable (i.e. list , data, array, etc)
• Check if a method name fits the function of it
• Do controller actions follow the RESTful architecture? (Don’t define show method to list all)
• Pay attention to the potential effect by modifying CSS style

Highly important

• When DB or model is modified, you must make sure it doesn’t cause an error
• Payment must not fail by errors from your side (not payment library or user side)
• Test security vulnerability (SQL injection, manipulated HTTP request)
• Every time make sure sign-up / log-in / inquiry forms work (opportunity loses)
• Check it actually send or post email / SNS to correct person on appropriate condition

Reminders before making a pull-request

• Briefly explain to the reviewer what this feature does and why
• Don’t request 1,000+ of code at once
• Split the whole feature into small features
• Name your working branch as what is clear to understand ( don’t name your name !)
• Don’t let your code unclear to you
• Carefully copy & paste
• …back-end: remove unnecessary variables/methods and test threshold value
• …front-end: carefully check class names
• Log your thoughts and discussions
• …server side file: use comment out
• …client side file(HTML, CSS, JavaScript): avoid commenting directly since users can see them, but note some other files. I prefer to put just <!--ref.1--> in HTML file
• …Github users: make use of issue
• Prepare seed generator file ( db/seed.rb ) for the reviewer to check functions
• …it is useful to have you DB open to internally public and share IP address with your reviewer, saving reviewer from migration and configuration

Wrap up

Code review is important. You’ll feel hard until you get used to it, but I believe it worth spending your time!!

Thank you a lot for reading all the way, I hope this could help you a little.😄

Yuta.

Building A Web Or Mobile App?

Crowdbotics is the fastest way to build, launch and scale an application.

Developer? Try out the Crowdbotics App Builder to quickly scaffold and deploy apps with a variety of popular frameworks.

Busy or non-technical? Join hundreds of happy teams building software with Crowdbotics PMs and expert developers. Scope timeline and cost with Crowdbotics Managed App Development for free.