Every single line of code in the Dropbox for iOS app began as a bug or feature added to Maniphest, our task management system. When an engineer picks a task, responsibilities begin before a line of code is written. Phabricator, the platform that contains our code review tools, has a lot of benefits, but it's not very good for evaluating interactions among objects at scale. To combat this, our engineers know who will be reviewing their tasks before they begin working. ¹ For reviewees, this ensures that they have a rubber duck on the team who has background on the reasons for the change and can assist with design decisions. For reviewers, this helps to break up the time spent evaluating a change into phases that occur over the project's lifecycle. Unsurprisingly, our experience has shown that upfront planning can help avoid iteration during review. Planning for the change can be as simple as a quick chat in front of a whiteboard, or as thorough as producing an architectural document. It's up to the team members assigned to decide what's best.

As work begins on the task, the engineer keeps our style guide in mind. It's a rather large amalgamation of best practices and consistency rules, but it exists to take the guesswork out of how we code and make reviewing code easier. ² Because the project is large, nobody on the engineering team has a perfect mapping or understanding of the entire app. So our engineer will rely on teammates to help piece together how functionality behaves, correlating conversations with the logic read in the code.

At some point while working on this task, our engineer will likely have to make a non-obvious tradeoff or unpopular choice. The best time to capture this reasoning is the moment it happens — in preparation for future explanation to the reviewer. To account for this being easier said than done, our engineers are encouraged to make use of //TODO, //HAX, and //FIXME comments in the project. //TODO and //FIXME are self-explanatory — though the latter generates a compiler warning and must be resolved before our next release. //HAX comments are where things get interesting. We typically use them to cite a non-obvious workaround to an Apple API. ³ Our comments are prefixed with the date and name of the person who wrote the comment, ⁴ and we're always thankful for this additional context. ⁵

As the development journey continues, our engineer will be tempted by spotting what looks like a quick improvement to an existing feature. Inevitably, this out-of-scope improvement will lead down a rabbit hole of realization that there are plenty of underlying consequences for making this "quick fix." This is a classic case of DoingTooMuch™. The only cure for our engineer is to file a new task for the improvement, and refocus on the assigned task.

If our engineer has gotten this far, hooray! The requirements of the task have been resolved. However, writing the code is but one aspect of this process. Now, the work begins to land the changeset.

Our engineer will use the command-line tool Arcanist to start the process of uploading the diff to Differential, our code review tool. During this process, scripts and unit tests will run. The scripts format our code, helping us focus on the functional changes of the diff and removing the cognitive load of stylistic nitpicks. Specifically, we use clang-format to enforce pointer spacing and line breaks, while a homegrown script auto-sorts our imports alphabetically. The nice thing about both of these scripts is that the changes are made like magic, but our engineer is able to double-check before committing the changes.

After the code has been auto formatted, existing unit tests will run against the diff. Of course, any failures need to be resolved by our engineer before proceeding.

Once the diff has been uploaded, but before it's been sent for review, our engineer has a few fields to fill out. First, an outline of the goals for the diff and how those goals were met needs to be written. Next, our engineer needs to attach a test plan. Our engineer did create a test plan, correct? Thought about all of the edge cases that could cause the code to break? Created a modular enough design to even be able to consider unit testing? If the answer to any of these questions is a bewildered "No," it's time for our engineer to close that Differential browser tab and open Xcode back up.

Now, with our engineer's well-thought-out test plan, the diff is ready to be submitted for review.

At this point, our focus moves to the engineer reviewer, who will work hard to give constructive feedback in a helpful manner. The use of memes will help. So will remembering that a fellow engineer, who is invested in the outcome of the diff and worked hard at solving the problem, is on the receiving end. Tone matters. Be nice.

Because our engineer reviewer has been involved since the inception of the task assignment, hopefully larger structural questions like "Is this code as modular as it could be?" or "Does this code avoid unnecessary duplication?" will be answered with "Absolutely!" If so, our engineer reviewer will dig deeper into the changes, completing a thorough evaluation that may include patching and trying out the change. Unless a change is obvious, rarely is a diff accepted without comment or a change request. So our focus moves back to the engineer who submitted the diff.

While reading the diff comments, our engineer will remember that the feedback received does not reflect on him or her as a person. Code is hard to get right on projects of any size, and it's particularly difficult on larger projects. Reviews facilitate a discussion between engineers that provide an opportunity for growth. A thorough code review process requires significant engineering effort, but it's emblematic of a culture that has effective communication.

After typically a few iterations of code review, depending on the size of the diff, our engineer's code is ready to land. ⁶ Bask in our engineer's prideful feeling that every single line now being added to the Dropbox for iOS app began as a task in Maniphest. Now, let's go pick another task.