Skip to main content

Code Reviews, Part 1

Why Code Reviews?

Code review is a practical and important skill that truly distinguishes the highly experienced engineer. Knowing what are the common pitfalls, where to look for them, and how to understand someone else's code quickly is an invaluable skill to have. It is also a skill that is rarely taught in school. To really ramp up your programming skills, get out there and read other people's code. Reading high quality code is a good learning experience. Occasionally reading poorly written code is also insightful. What are the most common errors and where do you find them? Through code reviews, software engineers can spread knowledge through their teams as well as improve consistency and code quality. For the interviewee, you have to review your own code, so it helps to know what are the things you need to double check. One great way to evaluate a team's culture is to probe and understand their code review practices or lack thereof. Below, I will cover some of the best practices and provide a few pointers on where you can get started.

One important thing is that all parties have to be conscientious to make code review work and to avoid the whole process turning into an adversarial process, which it definitely can. Code review shouldn't be something dreaded by both reviewer and reviewee. It ought to be a process beneficial to both.

Fundamentally, reviewing code is for the purpose of

  1. the reviewer understanding how the reviewee's piece of code works (so more than one person in a team will know how to maintain, extend, and support that code) and for the reviewee to understand any potential unexpected interfaces with other code
  2. for the reviewee and reviewer to exchange ideas about what is the best way to do things in code. The knowledge transfer is bidirectional. It both the experienced reviewing the junior and the junior reviewing the experienced.
  3. to get a second or third pair of eyes to find bugs
  4. to improve consistency and quality of code in a team
A team needs to have some ground rules for code review. Team mates should agree whether code style is in scope. When they are in scope, they can be identified as a nitpick (a nit).

How to Code Review

To prepare for a code review as a reviewee, it is best practice to briefly document:

  1. what your code does including what bug it fixes or what new functionality it introduces,
  2. what tests you have done on it and the outcomes
  3. how to run it if any nontrivial dependencies or build/startup instructions
  4. where to find your code if you aren't using a nice code review tool, you should package up your code as a diff using a diff tool
If at all possible, you should keep the unit of code to review to a manageable size, say 200 lines of C/C++ or Java code. The code review process should be a conversation. The reviewer supplies feedback, which he or she will expect you to act upon in a timely way. You should agree ahead of time roughly how long before a reviewer will get to your code after submission. Once you received the feedback, let your reviewer know when you'll address each of the points. Again, a code review tool can be helpful here to keep track of feedback and fix status. While you address the feedback, it may take a few times back and forth before you and your reviewer arrive at a mutually acceptable resolution. Both reviewer and reviewee will have to be patient here.

As a reviewer, here are some basic things to check for:

  1. Is there a real potential for an unhandled error or uncaught exception from a function call? The larger question is how error handling is done. Are errors being handled gracefully and in the right place?
  2. Are there race conditions in the code?
  3. Does the code match up with dependencies and client code in terms of boundary cases?
  4. Are there obviously asymptotically inefficient loops and other blocks of code? How can they be improved?
  5. What happens to this code if I use it in another relevant context? Do all the assumptions still hold?

Github is a veritable treasure trove of public code reviews. One learns best when one is the reviewee or reviewer, but it can help just by reading other people's reviews. Pay attention to what the reviewer is looking for.

Comments

Popular posts from this blog

Top 5 Books for Language-Specific Interview Questions

Shrunk and White of Programming When you put down that you know a certain programming language or languages on your resume, you are setting certain expectations for the interviewer. I would strongly caution against putting down "expert" in a language unless you invented or are one of the language's maintainers. You are giving your interviewer the license to quiz you on programming language lore. There are a handful of concepts that are considered "standard" knowledge for each language which go broadly beyond syntax and general semantics. These concepts commonly involve major pitfalls in a given language and the idiomatic technique for negotiating these pitfalls and writing efficient and maintainable code. Note, although the concepts are considered idiomatic, you can seldom infer them from knowledge of syntax and semantics alone. The tricky part here is that most courses that teach a particular programming language do not cover these idiomatic techniques and eve…

Interview Gotchas

It's a challenge to outperform all the other candidates in a competitive tech job only, but there is hope. You can improve your performance with practice and watching out for these gotchas: Make absolutely sure you are solving the right problem: I ran into this the other day. It is entirely a communication issue. When doing an initial screen over the phone, this problem is compounded. For example, maybe an interviewee is hacking out a function that returns the k highest priced products when the interviewer is expecting the kth highest priced product. One can squander a lot of time due to these misunderstandings. A good interviewer will try to guide you back to the right path, but you can't expect this. Be sure to ask questions. Confirm that the input and output are exactly what you expect. Use examples.Don't ever give an interviewer the impression that you are avoiding writing real code. This is an impression thing. This is a coding interview so you should be expecting to…

Complexity Analysis for Interviews, Part 1

This is part 1 of a two part series. Skip over to part 2 you'd like . For coding interviews, we are interested in gauging the asymptotic efficiency of algorithms both in terms of running time and space. The formal study of analysis of algorithms is called complexity theory, a rich field with fascinating and complicated math. For interviews, we only need a few basic concepts. Asymptotic efficiency is concerned with how the running time or memory requirements of an algorithm grows with the input size, so it is intimately concerned with how well algorithms scale to larger inputs. This is important in Computer Science and in practice because whereas some algorithms work well enough for small inputs of say < 10 inputs, the running time and space grows far faster than the input size and thus large inputs of say 10s to millions of inputs becomes impractical (usually meaning taking hours or even years of execution time). Consider sorting. Say for the sake of argument that sorting alg…