How much context is enough for effective code review?

Wednesday, May 22, 2013
imageMatt Gordon

Code review is all about context. Show me a clever, useful line of code and I’ll show you a place you can put it that will cause you to lose all faith in humanity[1].

Let’s look at an example. Here’s a controller action I made up because it’s convenient for me:

  def destroy
    @thinger = Thinger.find(params[:id])
    @thinger.destroy
    important_thing
    also_important_to_run_in_this_destroy_action

    respond_to do |format|
      format.html { redirect_to thingers_url }
      format.json { head :no_content }
    end
  end

Is this a good controller action or a bad one? It depends. Maybe it’s a single user system. Maybe you like wanton destruction. Whatever. I’m not judging you.

image

 Let’s say that you _do_ want to make it better, so you change 

@thinger = Thinger.find(params[:id])

to

@current_user_thinger = current_user.thingers.find(params[:id])

This is probably a good move, but how much context is necessary to be sure?

We talked a little about context in our post about branches when MadMan Mcgee tried to #make_do_terrible[2], but what about the context of a regular every-day change?

When you change a line of code, how much code should Codifferous ask you and your team to review? Just that line? The hunk? The file? The whole project?

If you voted for “the whole project” here, you are probably a robot. That’s ok. You can still have faith in your fellow man if you want.

Any amount of context less than the whole project _might_ cause you to incorrectly review a line. But, for those of us that are not robots, we think the hunk is best. The hunk will include local things like `important_thing` and `also_important_to_run_in_this_destroy_action` to help you make sure those still make sense, but most of the file (and project) will stay reviewed.

Minimum effort for maximum code review.

What do you think? Is the hunk too much code to review every time? Not enough?


[1] This article assumes that you still have faith in your fellow man.
[2] You should really fire that guy.