I’m an experienced code reviewer. It’s my passion and it’s probably what I enjoy the in my engineering career. I love it even more than writing code myself.
CR helps to maintain high quality software, to grow engineers, to force consistency across the source code and to have a chance to catch hidden technical debt before it’s too late.
Code reviews also can be used as a change management (CM) mechanism.
All human beings make mistakes, the absence of code reviews is a big sign demonstrating a manager’s neglect for their engineer’s personal growth and reluctance to avoid technical debt. I wouldn’t like working for you :).
I’ve sent thousands of code reviews myself, and have had the chance to work with the best engineers at both Microsoft and Amazon to review my own work. They encouraged me to grow, it was the best teaching tool for me besides reading books.
I’ve given many comments in over 2k-3k CR, from Windows to EC2 Console. I think I overall did 2 or 3k CR in my life as a reviewer. It is hard to quantify precisely, but Amazon’s repository shows 334 for this year alone.
For a typical CR (1.5 days of coding work max!), I invest at least 1-hour, often 2. Around 20% of my engineers’ time are dedicated on giving feedbacks to others. Same ratio at Microsoft, Facebook/Instagram etc..
System design AND high level design of objects/files/prototypes/[insert any] should have already been done during design review. We usually align on the design patterns we are going to use as well as the level of abstraction and encapsulation. We also agreed on the dataflow and data structures before the CR.
What I’m looking is algorithms and the way objects consume each other’s.
For every file, every class, every method, I ask myself a set of questions:
- Is your code correct? Are you accomplishing what you think you are trying to accomplish? It can happen that what your code is doing is actually not what you try to do. As a reviewer, I always start with the “why” before the what/how. Why did the developer make this change? And does that satisfy the business/technical requirements.
- Is your code robust? This is a big part! Usually it goes back to “is the engineer making assumptions? If these assumptions are wrong, what happens?”. Every variability in a program will trigger a thinking process from the reviewer.
What if this variable is null? What if this parameter is not the right type? What if, what if, what if!
Not only do need to think about variability but also HOW the program reacts to it. Do we have explicit exceptions and errors handling? How will the algorithm be fault tolerant and recover gracefully?
- Is your code readable? Do I understand your intentions? This goes back to the fundamental understanding that code is not only a message to a computer but also to another human being. Coding by intention by naming correctly, using the right amount of separation of concerns is a basic.
- Is your code extensible? In order to determine if a code is extensible, the first thing I will ask myself is if your code empower others. Is the code abstract enough, handles errors well enough, and is easy to understand to a point that other engineers can build on top of it? If that’s the case, then the next questions will be: what if the data structures/dataflows change, what would happen? If a new “type” is introduced, how will the code react ?
- Is your code testable? The best determinant is if there are tests written. And no, you will not have the “ship it” from me if the tests aren’t here :). Usually we look at basics things like mockability or granularity of the code.
- Is your code efficient? As I read the code, I compute time and space complexity. And think about performance and caching (caching in computer science is essential, from processor to your app, you have caching at all layers, without it your experience in the IT world would be drastically different.)
You could ask a lot of other questions related to domain knowledge: is this code parallelizable, portable, natural etc…
Code Reviews are NOT the place for design review. I use design reviews at the beginning of a sprint to align ourselves on what will be created. This is not the place to think about system design or business requirements. It is too late for those kind of things.
Do not nitpick things. Best way to be SUPER annoying. Put your ego to the side, we don’t care that you find a slightly better way to name this. It has to be a lot better. We don’t care if you think I need a ternary operator or an if (except if it is a team’s standard) etc.
Change management and Continuous Deployment (CD)
I also use code reviews as a change management mechanism.
My current projects are full CD, which mean if you checki-in your code will go through different environment up to prod. Your code will be facing and impacting real customers.
The first thing I do is a checkup on the things that needs to happen. We already aligned during design meeting, but as we know, mistakes can happen.
- If needed, is it documented?
- Is it tested? (integ, smoke, unit, …)?
- Is it localized? Is it accessible?
- Is it degrading performances?
- Is it secure? Cf STRIDE methodology
- Is it operational ready? Alarms, metrics, monitors, logs.
- If needed, is it regionalized?
Then after the code is clean, and the overall checklist is present in the code review, I will ask few remaining questions:
- Have you run all the tests (not only yours)?
- Have you make sure the logs and alarms works appropriately?
- Have you manual tested (all resolutions, all platforms, all devices…)?
- Have you try to do a full compilation, not just a partial one?
- Will you be available in the next hours to handle problems in case your check-in will introduce bugs?
- Do you need a weblab (A/B testing) mechanism around your feature?
- Do you have another engineer that also gave a ship it?
- Did you notify the DevOps on call?
And then, now, you can have my ship it and check-in your code 😛