This week in refactoring, I will be working on JRuby. Along the way, I will ponder the following themes:

• fixing broken windows
• the social nature of programming
• the cumulative benefit of making code readable

As usual, I began by downloading and building JRuby. The process was invitingly easy. I already have Jave, Ant, and Subversion, so I simply downloaded JRuby and ran the tests. Everything ran fine, and all the tests passed. However, there was one slight annoyance. The build output included these warnings:

  [junit] ./test/testStruct.rb:91 warning: redefining constant Struct::Post
  [junit] ./test/testStruct.rb:98 warning: redefining constant Struct::MyStruct

This is a classic broken window. Did these warning represent a real problem? My guess was "probably not", and a quick inspection of testStruct.rb proved my guess right. Still, such warnings must be fixed. These warnings add noise to the system, and each developer must mentally filter the noise out. I have been burned hundreds of times by ignoring warnings that didn't seem directly related to my task, so now my default assumption is that all warnings matter. Fortunately, I already had the code to solve this problem. Our training labs include lots of little console applications that are difficult to test because they do silly things like redefining classes, which is exactly the issue here. So I added this module

  module Recording
    def self.stdout
      $stderr = recorder = StringIO.new
      begin
        yield
      ensure
        $stderr = STDERR
      end
      recorder.rewind
      recorder
    end
  end

which makes it easy to swallow (and even test) those pesky console warnings:

  Recording::stdout do
    P2 = Struct.new("Post", :foo)
  end

I then submitted JRUBY-1249.

And now it gets embarrassing. I couldn't get the patch to work. Then I tried it in the MRI, and it worked fine. A hah! A difference between the MRI and JRuby, maybe a real bug worth worrying about. But I was tired, and something just didn't feel right. So I popped over to the JRuby IRC channel, and asked if somebody could take a look. Within minutes Nick Sieger pointed out that my idea was fine, I just needed to record stderr, not stdout. Doh! Armed with this new knowledge, I revisited the MRI/JRuby "difference" and realized it wasn't different at all. I don't know what my tired brain was thinking the first time I looked at it.

This is a great example of the social nature of programming. Two sets of eyes are better than one. Take advantage of as much code review, IRC communication, campfire, and pair programming as possible. The math is overwhelmingly in favor of collaboration. The cost of bugs and bad design in software far outweighs the ridiculous process optimization of having a developer code alone.

Once the compile was clean, I turned my attention to BigDecimal support. (I knew there might be some issues here based on a previous visit). The implementation of the to_s method caught my eye:

  int start = 0;
  int end = format.length();
  if(format.length() > 0 && format.charAt(0) == '+') {
      pos_sign = true;
      start++;
  } else if(format.length() > 0 && format.charAt(0) == ' ') {
      pos_sign = true;
      pos_space = true;
      start++;
  }
  if(format.length() > 0 && format.charAt(format.length()-1) == 'F') {
      engineering = false;
      end--;
  } else if(format.length() > 0 && format.charAt(format.length()-1) == 'E') {
      engineering = true;
      end--;
  }
  String nums = format.substring(start,end);
  if(nums.length()>0) {
      groups = Integer.parseInt(nums);
  }

The purpose of this code is to process any special formatting flags passed to to_s. Here's the Ruby documentation for the flags:

  If there is a '+' at the start of s, positive values are returned with a leading '+'.

  A space at the start of s returns positive values with a leading space.

  If s contains a number, a space is inserted after each group of that many fractional digits.

  If s ends with an 'E', engineering notation (0.xxxxEnn) is used.

  If s ends with an 'F', conventional floating point notation is used. 

The documentation is a lot easier to read than the Java implementation, but it doesn't have to be that way. I created some well-named helper methods:

  public static boolean formatWithLeadingPlus(String format) {
    return format.startsWith("+");
  }

  public static boolean formatWithLeadingSpace(String format) {
    return format.startsWith(" ");
  }

  public static boolean formatWithFloatingPointNotation(String format) {
    return format.endsWith("F");
  }

  public static int formatFractionalDigitGroups(String format) {
    int groups = 0;
    Pattern p = Pattern.compile("(\\+| )?(\\d+)(E|F)?");
    Matcher m = p.matcher(format);
    if (m.matches()) {
      groups = Integer.parseInt(m.group(2));
    }
    return groups;
  }

The helper methods are an improvement for several reasons:

• They are easier to test in isolation.
• They have intentional names.
• They minimize the amount of branching and comparison.

With the helper methods in place, I refactored the start of to_s like this:

  pos_space = formatWithLeadingSpace(format);
  pos_sign = formatWithLeadingPlus(format) || pos_space;
  engineering = !formatWithFloatingPointNotation(format);
  groups = formatFractionalDigitGroups(format);

All the operations are at the same level of abstraction, which makes this an example of the Composed Method Pattern. (For my money, Composed Method is worth most of the other Patterns put together.) I contributed the refactoring as JRUBY-1257.

I suspect that at least a few people will argue that my refactoring here is overkill, and that my time and effort could have been better spent elsewhere. To those people, I say this: The refactoring shown above eliminates a bug that causes to_s to blow up for some bad inputs. Did you see it while reading? Go back and see if you see it now. For me, it took refactoring and writing unit tests to catch the problem.

And remember, if this is your first week at refactoring club, you have to refactor. Go download JRuby and give RubyBigDecimal.java some love. There are several methods like this:

  public IRubyObject sign() {
      System.err.println("unimplemented: sign");
      // TODO: implement correctly
      return getRuntime().newFixnum(value.signum());
  }

If a few dozen people took one of these each, we could knock down a bunch of TODOs.