This week in refactoring, I will be working on JRuby. Along the way, I will ponder the following themes:
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:
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
which makes it easy to swallow (and even test) those pesky console warnings:
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:
The purpose of this code is to process any special formatting flags passed to to_s. Here's the Ruby documentation for the flags:
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:
The helper methods are an improvement for several reasons:
With the helper methods in place, I refactored the start of to_s like this:
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:
If a few dozen people took one of these each, we could knock down a bunch of TODOs.