Here is a good example of how not to submit patches

Original code (stripped down real example):

function f() {
  var x;
  // some stuff

  function g(y) { 
    // recursive helper function
  }
  foreach(x, g);
}

Personally I love JavaScript and Python because you can nest functions. You know those times you end up with a recursive problem and you have to write a new function? I hate that. I hate cluttering the class/module with a function that’s only ever called in one place, which only exists because of a syntax shortcoming. I love being able to embed that recursion within another function. It also works to embed helper routines that are called multiple times from only one function.

‘Patched’ code:

function f() {
  var x;
  // some stuff

  // This creates a closure which is slow and very dangerous!
  // I have moved this to another function
  /*
  function g(y) { 
    // recursive helper function
  } */
  foreach(x, function(e) { g(e); });
}

function g(y) {
  // recursive helper function
}

There are lots of problems here:

  1. Don’t comment out code you’re replacing. Either leave it there or remove it.
  2. Don’t add useless comments. Everyone qualified to edit it *knows* it creates a closure, it says so right there in the syntax of the language. The original author knew it too! Guess what: they didn’t care because the performance hit is not measurable or important.
  3. Don’t justify a refactor in comments. A comment referring to a historical state is useless. Code does not have history. Git has history. Justify changes in commit messages.
  4. If you’re going to refactor a closure to a separate function on the grounds that closures are ‘dangerous’, don’t create a new closure to invoke it, *facepalm*. Especially when it was already arranged to be an argument to forEach().
  5. Don’t call code dangerous unless it is. This isn’t. And stupid hyperbole disinclines me to take your opinions seriously.

There were other things too. Whenever I had a an i/j nested loop, he renamed j to ii. He did it all over. WHY? And then there were bizarre things like adding semicolons after blocks. if () {}; for () {}; and so on. Err, no. And another good one; I had a “FIXME” in a comment. He added a “FIXED” underneath it (amusingly, he didn’t actually fix it, but let’s assume he did), now the next thing I have to do is go through and remove both the FIXME and the ‘FIXED’. What. If you fix the FIXME then remove it. When I grep for FIXME it’s supposed to tell me stuff that needs fixing.

A useful patch does one or more of three things:

  1. Adds functionality
  2. Fixes bugs
  3. Rewrites old code to be cleaner/leaner/meaner/betterer

This does none.

Advertisements

I like blogging

Posted in Uncategorized

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: