Three Common Code Review Problems and How to Solve Them

I’m a big fan of code review, but it’s not always easy to get quality code reviews from my teammates:

  • They’re often too busy with their own code to look at mine.
  • Sometimes I’ll miss a number of small mistakes that block them approving my code.
  • They might not have all the context around the code I’ve written, and the code review prompts further discussion to clarify my approach and intentions.

Code reviews are too important to overlook, so it’s crucial to be as good at submitting them as possible. I’ve identified a few problems with code reviews, and techniques that have helped me overcome them.

Problem 1: My teammates don’t understand my code reviews – I have to justify my code.

The code review process is a cycle. You present some code, your teammates give feedback, you iterate based on the feedback, your teammates give more feedback, and so on until you get shipits / +2s / LGTMs / whatever. To get the most out of code review, you want to have as few cycles as possible.

Two easily avoidable factors that increase the number of these cycles are ambiguity (what does this code do?) and confusion (how does this code work?). You can minimize these factors by following these steps:

  • Be explicit. Write a great commit summary that explains the purpose of your review. Compare “This fixes JIRA-1234” to “Feature X broke under circumstance Y (see JIRA-1234) – this commit resolves it by doing A B and C.”. Your teammates will immediately understand your change, and can approve it faster.
  • Know your teammates, and teach proactively. Look at your review and try to emphasize with what your teammates might not immediately understand. Are you using a peculiar language feature, or is the design a little gnarly? In these cases, you should proactively leave inline remarks in the review tool explaining your choices. The more you get to know your teammates, the better you’ll know what to annotate.
    • Example: If you know your teammate has a hard time with JS promises, and your code review uses them extensively, add inline remarks in the code review explaining what you’re doing, or link to educational resources. This will save you a conversation about what the code means, and your teammates will approve your code faster.

Problem 2: My code reviews never get approved straight away – I make too many mistakes!

Code review isn’t a rubber-stamp formality – sometimes your code is going to fall short of the mark. That’s okay! Code review is when your code is assessed against the technical standards and norms of your team, and eventually you’re going to miss something, whether by omission or ignorance.

There are a few techniques I use to make sure the code I show my teammates is as good as possible:

  • Make a checklist. When I started my current job, I would often make similar mistakes in my CSS and PHP reviews. I eventually made what I called a “rigour checklist” as a reminder to check these frequent oversights, and my teammates had much fewer nitpicks after that.
    • For bonus points, automate your checklist. A properly configured linter or other script can catch 99% of issues that your checklist might encompass. For example, frontend engineers can use eslint to catch or automatically fix almost any JS style issue, and postcss is great for uniform enforcement of CSS style guidelines. There are similar tools for almost every language.
  • Be self-critical. I like to do a “self-review” of big patchsets before I ask my teammates to look. I get fatigued when I spend a lot of time in the editor, but my brain switches gears when I look at the same code in GitHub or Gerrit. My perspective is refreshed, I’m more likely to notice mistakes, and I can catch problems before my reviewers do.

Problem 3: It takes too long to get reviews!

Your teammates are just like you – extremely busy, protective of their time – so it can take a while to get the reviews you need. Some tips:

  • Be forthcoming with your reviewers. Posting a URL in the team Slack channel with a brief description is a start, but the personal touch is best; ask each teammate for reviews, particularly if they’re experts in the code you’ve touched. This way they’ll be much more likely to respond. However…
  • …the flip side of being more forthcoming is that you must be respectful. Treat your teammates’ time as you’d like them to treat yours. If your teammates know your patchset’s purpose, urgency, and length, they can budget how long it’ll take to review, plan accordingly, and more accurately set your expectations. If they’re an empathetic engineer, and you follow these steps, they won’t begrudge your request.

Further Reading

In Conclusion

Code review isn’t perfect, but it’s better than the alternatives. Following these tips will help your teammates get the most out of code review, and may even make it a delightful process.

Thanks to Nick Morgan for providing feedback on an early draft of this article, to Si Pham for recommending almost everything in the “Further Reading” section, and to my fabulous teammates for giving great code reviews week after week.

Quick bugfixing with git bisect

git bisect is an indispensable tool for fixing bugs. It can be an extremely efficient way to identify a change that broke your code and therefore find a fix for your problem.

git bisect does its magic with a simple binary search. You start by specifying a commit where the code is broken (with git bisect bad) and a commit where the code works (git bisect good). git bisect will automatically check out a commit between those two commits. You test the code again, mark it as good or bad, and bisect will check out another commit. Eventually, you’ll narrow down to the commit when the code broke, and then you can fix the bug yourself (or identify who else broke the code, and get them to fix it).

My favorite way to run git bisect is with the run parameter. If you have a script ./script.sh that can test if the code is good or bad, and it exits with 0 for good and anything else for bad, then you can use git bisect run ./script.sh – it’ll automatically use the script’s output to mark commits as good or bad, and will quickly identify the breaking change.

There are a lot of finer points to using git bisect, as detailed in the official documentation, and I’ve glossed over some details, but I’ve never needed more than this basic usage. I hope it’ll make your coding life happier. Enjoy!

HTML validation with vnu

vnu is an HTML validation checker. It’s extremely easy to use and integrates well with all your other command-line tools. I installed it with brew install vnu – it’s also available through npm and pip.

I like to use it in conjunction with cURL, so I can quickly check the validation status of whatever I’m working on:

curl https://example.com | vnu -

I prefer the JSON output format, because I can pipe it into jq as follows:


// curl ENDPOINT | vnu --format json - 2>&1 | jq
{
  "messages": [
    {
      "type": "error",
      "lastLine": 3,
      "firstLine": 1,
      "lastColumn": 70,
      "firstColumn": 1,
      "message": "Start tag seen without seeing a doctype first. Expected “<!DOCTYPE html>”.",
      "extract": "  \n\n        <h2 class=\"popup-heading\" title=\"Blah Blah\"> Embed",
      "hiliteStart": 0,
      "hiliteLength": 74
    },
    {
      "type": "error",
      "lastLine": 3,
      "firstLine": 1,
      "lastColumn": 70,
      "firstColumn": 1,
      "message": "Element “head” is missing a required instance of child element “title”.",
      "extract": "  \n\n        <h2 class=\"popup-heading\" title=\"Blah Blah\"> Embed",
      "hiliteStart": 0,
      "hiliteLength": 74
    },
    {
      "type": "error",
      "lastLine": 3,
      "lastColumn": 1583,
      "firstColumn": 1559,
      "message": "Element “div” not allowed as child of element “label” in this context. (Suppressing further errors from this subtree.)",
      "extract": "n\"><label><div class=\"input-title\"> Width",
      "hiliteStart": 10,
      "hiliteLength": 25
    },

If your HTML is returned from a JSON endpoint, use another jq call to extract it from the JSON:

CURL COMMAND | jq -r .data.html | vnu --format json - 2>&1 | jq .messages

By default, vnu expects full HTML pages, and as such will complain if it’s asked to validate HTML fragments. If you’re validating HTML fragments like in the above examples, you’ll probably see messages like Start tag seen without seeing a doctype first. Expected “<!DOCTYPE html>” or Element “head” is missing a required instance of child element “title”. Don’t worry if this happens – these are only problems if you’re trying to validate a full page.

Combined with your other tools, vnu will help you fix your HTML quickly and efficiently. Enjoy!

IntelliJ Task Management: deal with your tickets like a pro

When I submit a patchset for review, I don’t shut down and wait for the requisite shipits to roll in – I move on to the next item on my backlog. Unfortunately, I’ll probably need to go back to the files in that patchset and act on my peers’ feedback, which means I’ll have to clean up my workspace and reopen the files I had earlier. I’ll want to have my editor split, with the code on one side and the tests on the other, and when I’m done addressing the feedback, I’ll have to clean up again, start the other task’s work again, and cross my fingers that I don’t have revisit that original patchset.

Switching between tasks is taxing, but lucky for us, IntelliJ has Task Management built in. Here’s how you set it up, get started, and make your life easier.

  1. Hit cmd-shift-A and type in “Configure Servers”. There are multiple “Configure Servers” actions, so select the “Tasks & Contexts” one.
    Screen Shot 2016-08-24 at 10.17.02 PM
  2. Hit the + icon and select the issue tracking server where your bugs live. For this example, I’ll use a project on my personal GitHub account, but as shown below, all the major issue tracking systems are supported (I usually use my work’s JIRA instance)
    Screen Shot 2016-08-24 at 10.18.46 PM
  3. Enter your repo credentials and details, test it out, then hit OK.
    Screen_Shot_2016-08-24_at_10_20_36_PM
  4. Now the fun starts. Hit option-shift-t, and the task switcher window will appear. Make sure “Open Task” is selected, and hit Return.
    Screen Shot 2016-08-24 at 10.24.13 PM
  5. Voila! All the open issues from your GitHub project will appear. Select the bug you want to work on and hit Return.
    Screen Shot 2016-08-24 at 10.25.03 PM
  6. You’ll get a few options on how to set up your workspace. I like to start each branch with a clean slate, so I select “Clear current context”, but uncheck it if you’d prefer to keep your current files open. With the “VCS Operations” checkboxes, you can switch to a new or existing branch for your code.
    Screen Shot 2016-08-24 at 10.26.49 PM
  7. Hit “OK” and do your work as normal. If you selected “Clear current context”, all the files that were previously open will be closed.

From now on, whenever you switch tickets, make sure to do “Open Task” from step 4. When you switch back to a ticket you worked on previously, IntelliJ will reopen the files that were open last time the ticket was active. They’ll be right where you left them – it’ll even remember if you had multiple panes open. No more tedious cleanup and setup!

This approach has made context-switching between tasks almost painless for me, and as a result of adopting it I have considerably increased my productivity. If you can relate to the frustration in the first paragraph, I highly recommend you try out the IntelliJ task management functionality. Enjoy!

3 Awesome Ways to Use Pycharm/IntelliJ/Webstorm Live Templates

The IntellIJ family of IDEs (IntelliJ IDEA, WebStorm, RubyMine, PyCharm etc) all support Live Templates, which I find extremely helpful to my work. In JetBrains’ own words:

Live templates let you insert frequently-used or custom code constructs into your source code file quickly, efficiently, and accurately.

In summary: you type a shortcut, you press your “expansion key” (usually tab), and the template expands. Here are three ways I use Live Templates to boost my productivity.

Conforming to coding standards

My team prefers our mocha test cases be written in a “should… when” style, ie it('should X when Y', function() { /* stuff goes here */ } ). I’ve been rumbled in code review a few times on this, so I made a Live Template that helps me adhere to the standard. Here’s how it looks:


it('should $SHOULD$ when $WHEN$', function() {
 $BODY$
});

I’ve abbreviated this block so that whenever I’m in a JS file and I type it followed by tab, it expands as follows:

Notice that when I hit tab, it goes to the next placeholder in the template. Easy!

Fast, sloppy debugging

Before I discovered this next technique, I would pepper my serverside code with debug statements like var_dump('here'); or var_dump('foo'); while trying to diagnose an issue. Coming up with unique dumb strings for var_dump gets tricky after the first ten, so I decided to let IntelliJ name them for me.

In this next example, I take advantage of IntelliJ’s ability to run Groovy code in Live Templates. I don’t know Groovy at all, but it’s a JVM language, so you can use the standard Java library. The template, which I have abbreviated to vvaarr, looks like:

var_dump("$RANDO$");

In the “Edit Variables” pane, I’ve set the $RANDO$ variable as follows:

groovyScript("String.format('%X', new Random().nextLong())")

This generates a random number and formats it as a hex string. The result looks something like var_dump("E41C2F2794389D6B"); Here’s how it looks in practice:

Now I don’t need to think about the strings to put in my debug statements. I can hit vvaarr-tab all over my code, look at the program output, and immediately connect the breakage to a location in the code.

(yes, I know I should set up a PHP debugger…)

Surrounding code

This technique takes advantage of Live Templates’ reserved $SELECTION$ variable, which represents whatever is currently selected in the editor. Sometimes when I write JavaScript I want to wrap a block of code in a function, so I can reuse it elsewhere. This is super easy with Live Templates. Here’s what the template looks like:


function $FN_NAME$ () {
$SELECTION$
}

In action:

Don’t forget to set “Reformat according to style”, so that the block is indented per your settings.

In conclusion

I hope this post has given you some inspiration on how you can use IntelliJ Live Templates to make your coding life more productive. Enjoy!

Efficient endpoint testing with curl, jq and pup

Sometimes I need to work on web pages whose HTML is dynamically loaded from a JSON endpoint. There’s no better way to verify the module’s behaviour than manual testing, but much of the time, I just need to ensure the correct HTML is rendered. It’s slow to click around the site, so I use curl, but since the HTML is inside JSON, it isn’t feasible to interpret its output.

Fortunately, it’s easy to break this HTML out of its JSON cage. I use two excellent tools, jq and pup, for this kind of work.

Say the JSON endpoint’s output is structured as follows:


{ "data": {  "html" : "<div><h2>whatever</h2><span>hey</span></div>" } }

First, I go to the page I’m working on and open Chrome’s network tab. I manually perform the task that hits the JSON endpoint of interest, I right-click on the relevant call in the network tab, and I select “Copy as curl“, which copies all the relevant cookies and headers necessary to replicate the request.

The curl command will be piped into jq and pup as follows:


curl ((ARGS GO HERE)) | jq -r .data.html | pup -p

The jq call extracts the HTML from the JSON and the -r flag removes the outer quotation marks. The pup call formats the HTML and the -p unescapes the output (otherwise it’ll be full of entities).

The output will look like:


<html>
 <head>
 </head>
 <body>
  <div>
   <h2>
    whatever
   </h2>
   <span>
    hey
   </span>
  </div>
 </body>
</html>

You can use CSS selectors to filter the pup call as follows:


$ cat output.html | pup h2

<h2>
 whatever
</h2>

If you want to take your productivity up another level, you can use this combination with watchman-make as described in this post: watchman-make: focus on your code.

jq and pup are very powerful tools, and even though this example is probably the simplest thing you can do with them it can really accelerate your endpoint testing. You can install both tools through Homebrew. Happy coding!

watchman-make: focus on your code

One of my favourite software development tools is watchman-make. From its homepage:

watchman-make is a convenience tool to help automatically invoke your build tool in response to files changing. It is useful to automate building assets or running tests as you save files during development.

In short: you change your files, and watchman-make runs the command of your choice. I don’t like to switch away from my IDE as I code, so automatically re-running my test commands helps me stay productive and focused. It’s also extremely easy to configure.

Here are some examples of how I use watchman-make.

  • Automatically run JS tests when a JS source file, test, or fixture template changes (aliased to watchjs)
    
    watchman-make -p \
      --'js/**/*.js' 'test/**/*.js' 'test/**/*.tmpl' \
      --make 'npm run -s' \
      -t 'changed-files-test'
    
  • Automatically sync CSS changes to my development server (aliased to watchcss)
    watchman-make -p '**/*.css' --make work -t sync

    (work sync is an inhouse sync command at Box)

  • Run a designated phpunit test file when I change a PHP file (aliased to watchphpunit)
    watchman-make -p '**/*.php' --make ./phpunit -t $@

One shortcoming: watchman-make expects the commands it runs to have a target, eg make install or npm test; it errors if you just set a make parameter and no target parameter, eg ./run_all_my_tests. Fortunately you can bypass this by setting the target parameter to '' (the empty string). (If you’re feeling motivated, I filed an issue about this on the project’s GitHub and the maintainers said they’d be amenable to a PR)

Installation instructions for watchman-make are available on the project’s GitHub. Happy coding!