At Tivix we iterate on ideas fast, we like to Get Shit Done! A big part of that is doing code reviews before features are pushed to the application. This blog post is specifically about catching problems with the web development process, but should apply to mobile or native development as well.

Why?

It’s important to do code reviews for:

• Knowledge transfer – “Oh wow, I didn’t even know that was possible!” or “You could do this 10 line thing in 1 line” aren’t uncommon.
• More eyes on the prize – Confirming the spec was met is important, a small oversight can be annoying for the client and cause a significant amount of overhead to re-do parts of a feature.
• Putting out fires – Catching potentially disastrous code before it becomes a problem.
• Building relationships – Often times during a code review you can get a good sense of a person and that can improve how you interact with them. Maybe they are sensitive about their code, a couple “Nice solution!” comments can go a long way when you may need to criticize a bad practice or large oversight. Try not to put people on the defensive or be accusatory.

Overall process

A mile high view of the code -> pull request -> merge process:

1. Get a spec from the client
2. Start working on the spec in a new branch feature/my-new-feature
3. Create a pull request
4. Get code review
5. Make fixes from code review
6. Push code to production

Pre-code review

• Lint your code (code style checker) – It’s kind of a waste of time in a code review to comment on syntax problems, this should be automated away by tools like flake8. You could have it as part of your development process through your IDE or a git commit hook.
• Automate away common problems – Often times we consistently run into the same problems on projects, misspelling “neighbor” or transposing lat/lon arguments is easy to do and hard to catch in a code review. A tool to look for these problems will help everyone sleep better at night!
• Smoke test – You should write at least the bare minimum tests, even while features are being iterated on. The bare minimum for a Django app might be to make sure the views return 200, or 403 if a user doesn’t have access to that page. A better smoke test might also use selenium to verify a couple more details about the page.
• Check yourself – Before you wreck yourself, you should at least do a once over of your code–are there any prints or console.logs left in there? Maybe that’s a good spot to add some logging?
• Test the site – Run through a few pages of the app quickly if you don’t have great tests, maybe something small was affected by this feature?
• All tests passing – Before a code review all of your tests should be passing and merged with the latest development branch.

Useful tools

flake8 – Checks pep8 and determines McCabes complexity which may not be the best way to determine if your code will be error prone, but it’s better than nothing!

equals – Increase entropy of tests with random data.

any_string.containing(‘abc’) == ‘123 abc 456’

model_mommy – Similar to above but with models instead of simple data structures.

github checklists – Helpful when you have fifty thousand things to do in one pull request.

Continue reading Part 2!