Code reviews are good. You should do them. In this post I’d like to expand on one paragraph from that post:

Don’t do gigantic code drops. If it’s a big feature, map out smaller steps towards the final goal. Mark unfinished places with TODOs so that the reviewer knows this is not the final thing. Build trust in each other to understand that sometimes the road to the final feature takes multiple steps. But always keep an eye on those first couple of steps to make sure that the road is taking the right overall direction.

Gigantic code drops happen every once in a while, especially with new people on the team when they start working on their first feature request. People kind of drop off for a few days, and then they come back with this CL with 1000+ lines of code that span over a couple dozen new and existing files. And that’s very unproductive for everybody.

Some feature work is naturally big, and will eventually indeed span that many lines of code and that many files. But you shouldn’t be trying to do it all in one take. When you’re doing too many things at once, you focus on none, and everything suffers equally. You power through everything from beginning til end, from bringing the data to displaying it on the screen to handling rotation to caching things locally to pixel-perfecting the layout to animations and transitions. And then you add your reviewers. Let’s pretend I’m one of those reviewers.

So I get an email saying that there is a new CL for me to review. I click through and I see that it’s a 1000+ line drop. That’s your first barrier right there. It’s just too much stuff for me to look at right now. Maybe later in the day when I have nothing to do? Because I’d need something like half an hour to take a serious look at it. And it’s not like I just have that half an hour kicking around in my day. I can certainly find 5-10 minutes for a 100+ line review, or even 10-15 minutes for a 200+ line review. But a 1000+ line review? That would need to wait.

And then I finally realize that it’s been sitting in my queue for a while now. And I take a look. And it’s just too much. There’s just too much going on at the same time, from network to caching to persistence to pixels to animations to transitions. I wasn’t there in your head as you were writing it. So now I have to unravel the “roadmap” of this code drop, step by step. How many layers does it have? What is the first step in that roadmap? What is the foundation of that roadmap and your overall direction? How do I identify and compartmentalize each individual layer so that I can intelligently comment on it?

And what happens if I see something that should be reworked completely in one of those foundation steps? How is that going to ripple through the rest of your CL? What are the chances that in order to address the comments in just that one layer you now need to effectively rewrite the whole thing from scratch?

And what happens if me and this other reviewer are having a discussion on this one layer, and there’s a parallel discussion happening on another layer? If you have everything in one big code drop, it’s hard to keep track of what is being discussed, what decisions are being made, and how these decisions need to be translated to revising the code.

It’s perfectly fine to break it into steps. You might not be sure what are those few couple of steps to take at the very beginning – without feeling compelled to travel the whole road. That’s when it’s time for a short discussion. If you know who you’re going to be adding as your reviewers, grab them for a few minutes at their desk or over IM. Talk over the feature and what it involves in your opinion. Describe the steps you’re planning to take to get there, with special emphasis on those few first steps. See if those are the right steps to take. You might “waste” a few extra minutes talking it through, but it’s nothing compared to the hours of your teammates’ time being spent on reviewing that gigantic blob, and to the days of your own time being spent on rewriting that gigantic blob. Possibly more than once.

Break it apart. Define your data structures and server endpoints first. If the server side is not ready yet, create some fake data locally, but try to keep it as closely to the eventual communication protocol as possible so that it’ll be easy to swap that out for real data. Then get something on the screen. It doesn’t have to be beautiful. It needs to be functional so that people can get a rough feel how the data flows on the real screens in their hands. Up until now it’s been pretty sequential. Now you can parallelize the work, either as multiple CLs from you, or multiple people pitching in.

Do a separate CL for local data caching. Do another one for data and view persistence and restore as you rotate the screen, interact with the content around your new bits or go out of your app and back in. Do a separate CL for pixel-perfecting the visuals, attaching screenshots and sending local builds to people to take a look how the static mocks done on gigantic desktop screens actually look and feel on smaller mobile screens. Lay down the ground work for adding animations and transitions, and do those in separate CL(s).

I’d say that an ideal CL is not more than 200 lines of code. Some just have to be more and that’s fine. But if you can keep it under 200 lines or so, you’re making it that much easier for everybody to engage in a productive discussion and to move the process forward. And you’re spiraling ever closer to the final state instead of just going in circles. Funny thing about some spirals though. You might be getting closer but you’ll never quite get there. But that’s a whole other topic.

When you break it down into multiple steps where every step needs to be reviewed separately, it might feel that it’ll take much more time compared to just powering through everything and doing one gigantic code drop. Trust me, it doesn’t. And at some point it just becomes a second nature. You start to break everything into small, manageable steps, and focus on each one of them. And before you know it, the feature is done and ready to ship. And that’s when the really scary bugs start happening.

Sometimes it feels that you simply can’t win. No matter how many bugs you slay, no matter how fast you crank out those features, your incoming queue never quite dries up. It’s easy to get lost, and it’s hard to see that you’re making a dent.

I’ve been thinking recently that I can easily take three months off, forget that incoming queue and just work on stuff that I’ve been neglecting. Things that can been languishing in the queue, not quite ready to fall off the cliff, but not quite important enough to elbow their way to the top. Things that once were marked as launch blockers, and then when the launch time was getting near, turned out to be actually just nice to have.

Three months feels just right. It’s a good chunk of work that I can tap into every now and then when things get just a tad quiet in the regular release schedule. But it’s not so overwhelmingly big that it makes you think that you’ve sacrificed too many other things at the altar of featuritis.

Of course, I’m rarely in charge of my own time. I don’t quite envision a near future where I can just go off for three months, chipping away at things that I want to do. A fiercely competitive market is a wonderful thing. You can spend all your time tinkering at the edges. Or you can stay focused on your core, falling back to the edges every now and then. Just don’t lose track of which is which for you.

In the interview I recently published on this site, production designer John Lavin talked about separating yourself as a person from things that you work on:

I used to teach drawing, and just the exercise of taking somebody who is not really used to that and saying “Here’s the drawing you made, go hang it on the wall and we’ll discuss it”, putting things up for criticism is something that I got pretty used to. And even though I’m sensitive to the criticism as a person, I’ve got a lot of practice with the idea that this painting, or movie, or thing that I work on isn’t me. It’s a thing I made, but it’s not me. It’s separating my identity from my product.

In Creativity, Inc. Ed Catmull writes this in the chapter “Honesty and Candor”:

[…] the only way to get a grip on the facts, issues, and nuances we need to solve problems and collaborate effectively is by communicating fully and openly, by not withholding or misleading. There is no doubt that our decision-making is better is we are able to draw on the collective knowledge and unvarnished opinions of the group.

And in the last post I talked about how important it is to focus on criticizing the code and not the person behind it:

Stay focused on criticizing code. If you feel strongly about something, be direct and concise. But never ever ever get personal. See the arguments for what they are and for what they should be. You are not your code. A critique of your code is not a critique of you as a person. Sometimes it’s hard to tell the difference especially if you’ve been working on something for a long time.

You can call it honesty. You can call it candor. You can call it sincerity. You can call it frankness. Some of these words come with heavy baggage attached to them, and that baggage can vary based on the cultural and societal aspects that you have absorbed in your life.

Clarity is the most important part I strive to achieve in the code I write. Clarity also plays a critical part in the code review phase, no matter which side of the process you fall on. In fact, let me strike that. In the code review phase it should not matter who wrote the code and who is reviewing it. You should work together as a team to crystallize the best solution possible to implement a specific feature or to fix a specific bug.

Communication within a team of people is already complicated. We all bring our own baggage, our own upbringing and our own cultural environment to it. Communication is by definition a two-way channel. Removing the complexity from this channel is the responsibility of both sides of the channel. Bringing clarity and candor to this channel is the responsibility of both sides of the channel.

As the person being requested to do code review, you should free yourself from the notion of how “senior” or “junior” the code author is. Bad code is bad code. Complex code is complex code. Criticize the code, fairly and constructively. The down side of not being as “senior” on the team is that you might not be aware of all the legacy aspects and decisions that were made when you were not on the team yet. It’s the responsibility of the code author to point you to the relevant decisions when you ask about them. The up side of not being as “senior” on the team is that you bring a fresh look. A fresh set of ideas. A fresh pair of eyes that should look at the code critically and see where, when and how it can be improved instead of being left to slowly stagnate.

As the person writing code and submitting it to review, you should detach yourself as a person from the product you are making. Free yourself from being defined by the code you write. Do not judge review comments based on how “senior” or “junior” that reviewer is. Free yourself from looking at those comments as something that originated from a person and treat them as a thing of its own. Some things that might be very clear to you would be too dense and opaque for a person who is not familiar with that part of the code base. Does it need more documentation? Does it need splitting up the logic into smaller, more manageable pieces? Or perhaps you unwittingly have fallen prey to the complexity beast?

The up side of being a “senior” on the team is that you are more aware of all the legacy aspects and decisions that were made since you’ve joined the team. It’s your responsibility as the code author to point the reviewers to the relevant decisions when you are asked about them. The down side of being a “senior” on the team is that it’s so easy to fall into the trap of been-there-done-that. It’s when the code base is slowly crumbling around you under its own weight, but you don’t even realize that it’s happening. That’s when you need somebody that brings a fresh look. A fresh set of ideas. A fresh pair of eyes that would look at the code critically and see where, when and how it can be improved instead of being left to slowly stagnate.

And if all the members on the team are candid and fair in code reviews, you might all just have a chance of enjoying working on that code base. At least most of the time.

One of the most enjoyable, and at times scary, things about working on my team is doing code reviews. They are mandatory no matter how “senior” you think you are or how long you’ve been on the team. I sometimes catch myself thinking “better not add Mr. SoAndSo because he’s going to point out all the ugly things in this pile of hacks I’ve been constructing here”. It’s those times where I just go back and try to make things less hacky. And then add that guy who would have the most objectively critical eye to look at the code change.

Do code reviews. You’ll occasionally butt heads, as there’s rarely a single “best” way to make even the simplest bug fix. But no matter how “senior” or “junior” your reviewers may be, you’ll always be better off at the end. I lost count of the number of times that I said to myself “Huh, I didn’t know that” after seeing a reviewer leaving his comments. Not to mention that you’ll have somebody else having at least some level of familiarity with your code if you ever get too swamped. Don’t count too much on that though, as people weave in and out of projects.

Every once in a while you’ll get out of your comfort zone working on a new feature or fixing a bug. At some point you know your reviewers, their style and their strengths. Don’t be tempted to add reviewers that will be inclined to rubber stamp such code. Seek out people who are deeply familiar and well versed in the specific area. You will be afraid that they’ll pick your code apart like a poorly constructed house of cards it is. Better now than after it ships. Such a reviewer knows how to do this better than you, and the result will be better for both you and your code base. It will just take longer. Don’t let your ego take a hit. It’s just code.

When you’re asked to do a review, do it quickly. Don’t drop everything and do it immediately, but do it the same day. The quicker you provide good and actionable feedback, the fresher it is on the side of the code author. Don’t let your reviewers rot. This also helps preventing unnecessary rebases. Not to mention spreading your strengths around, and learning from the strengths of others.

Don’t do gigantic code drops. If it’s a big feature, map out smaller steps towards the final goal. Mark unfinished places with TODOs so that the reviewer knows this is not the final thing. Build trust in each other to understand that sometimes the road to the final feature takes multiple steps. But always keep an eye on those first couple of steps to make sure that the road is taking the right overall direction.

Stay focused on criticizing code. If you feel strongly about something, be direct and concise. But never ever ever get personal. See the arguments for what they are and for what they should be. You are not your code. A critique of your code is not a critique of you as a person. Sometimes it’s hard to tell the difference especially if you’ve been working on something for a long time. Code reviews are a negotiation. It’s not a zero sum game. In the end, it is all about clarity and complexity.

There’s no better opportunity than doing code reviews to grow and become just that much better. Every single day, if you can. Even after all these years. There simply is no such a thing as those mythical 10,000 hours in our field.