Jake Scruggs

writes ruby/wears crazy shirts

Our team has been doing a bang up job of reducing our complexity through our hit list (All of our methods are ranked by Flog score and we spend some part of every iteration picking the worst methods and trying to refactor them.). But sometimes we run into a situation where we prefer a high Flog score to a low one. For instance, this bit 'o code:

def is_something?(x, y)
x.foo == y.foo &&
x.bar == y.bar &&
x.blah == y.blah &&
# and 10 more lines of the same

which had a Flog score in the 60s. We could have changed it to this:

def is_something?(x, y)
["foo", "bar", "blah", # and so on ].inject(true) do | a, b |
a && x.send(b) == y.send(b)
Which gave a us a much lower score. However that first method is crazy simple. Merely glance at it and you know what it's doing. The 'less complex' method? Even an experienced ruby dev would need a moment or two. As for a new dev... I can remember a time not so long ago when I found an inject in some source code and was pretty confused even after I looked up the Rdoc. It's one of those methods that's so powerful and flexible that the documentation only seems to make sense after you understand it.

After some discussion with the team we decided that while Flog is very good at telling you which methods to inspect for badness, it is not always true that a better score makes a for better method. The same can be said for all metrics, really.

On my last project we did some javascript unit testing in browser using the unittest.js library from scriptaculous, but because it's kind of a pain to set up and integrate into your build, I didn't get it working on my current job. Which makes me a bad person. However, Dr Nic Williams has made yours and my life easier with his javascript_test plugin for rails. Now I can type:

ruby script/generate javascript_test fancy_javascript_file

And get a fancy_javascript_file_test.html to test my fancy_javascript_file.js. It's all set up to go right at the javascript file. Just open the html file in a browser and it runs the tests. The sweet doctor explains it all better than me so go check out his site.

What I want to talk about is trying to integrate this into your Cruise Control build. The first problem we faced is that the tasks that come with the plugin don't close the tabs created in the browser. So after 10-20 builds, you're going to have way too many tabs and some memory problems. My teammate Toby Tripp came up with this:

def teardown
applescript <<-EOS if macos?
tell application "System Events"
tell process "Firefox"
set frontmost to true
click menu item "Close Tab" of menu "File" of menu bar 1
end tell
end tell

This code snippet goes inside

class FirefoxBrowser < Browser

inside the plugins/javascript_test/lib/javascript_test.rb file. We could have just killed Firefox but we needed to play nice in case any other builds were using Firefox at that moment (we test with Selenium to use Firefox to click through the app).

Now all you have to do is change the position of the browser.teardown call in that same file in the 'define' method. By default, teardown is called just before the end of the block created by

@browsers.each do |browser|

But I want it to get called after every test so you can put it just before the end of the block created by:

@tests.each do |test|

So that the end result looks like this:

def define
trap("INT") { @server.shutdown }
t = Thread.new { @server.start }

# run all combinations of browsers and tests
@browsers.each do |browser|
if browser.supported?
@tests.each do |test|
browser.visit(" http://localhost:4711#{test}?resultsURL=http://localhost:4711/results&t=" + ("%.6f" % Time.now.to_f))
result = @queue.pop
puts "#{test} on #{browser}: #{result}"
@result = false unless result == 'SUCCESS'
puts "Skipping #{browser}, not supported on this OS"
# browser.teardown

You can see by the comment where the teardown used to be. So now when you run your test the tabs will close when finished. In my next post I'll talk about how to fail the build when the javascript tests fail.

Oh, and I'd like to thank Mike Ward for turning me on to javascript unit testing in the first place.

Weird requirement at work today: We needed a number that was alpha-numeric, exactly 11 digits, unique, and non-sequential. At first we thought of using a hash, but MD5 and Sha1 give you way too many digits. We could truncate to 11, but not knowing much about hashing, that made me pretty nervous that we'd have collisions. After a bunch of discussion, we decided on this:

MAXIMUM_FOR_10_DIGITS = 0xffffffffff
(MAXIMUM_FOR_10_DIGITS + cart_id + Time.now.to_i + rand(100_000_000)).to_s(16)
I was thinking about using hex, but I was worried about what would happen if the number got too big and went to 12 digits. So I did this calculation:

irb(main):001:0> 0xfffffffffff - 0x10000000000
=> 16492674416639
Turns out there's all sorts of room between the lowest and highest 11 digit hex number. Paul and Schubert did some quick calculations assuming lots of carts per day and figured out that we have about 250,000 years before we run out of space. Now since we use a random number, there is a chance that we could get two matching numbers so we do a database lookup to make sure that a generated number hasn't been used before. There is still a small chance that two identical numbers could be generated in the time between generation and saving to the database, but I think we can live with that.

Update: After some interesting comments, I wrote a follow up post. Check it out for more math goodness.

In a previous post I talked about how my team has a daily metrics build that reports on our code quality. One of the things we measure is the Flog score of our methods (Flog is a ruby program that evaluates the complexity of your methods). We have a standing developer task to look at the top five worst methods in our application and reduce their complexity. Every iteration we try to spend some time looking at a few developer tasks, which are things that don't provide any direct business value but do make our code better, automate some manual process, or fix some technical problem not tied to a bug.

Yesterday Chirdeep and I refactored the worst method in our app and while it wasn't easy it produced a number of benefits:

First, about halfway though the refactoring we came upon some twisted logic that seemed as if it could never evaluate to false. After pulling in Ali for some consultation, we realized that indeed it would always be true and therefore was a bug. We fixed it and moved on.

Second, we noticed that the object inherited from another object, but overwrote the constructor with a strange one that took different arguments. This had the effect of making other objects interact weirdly with this class in order to deal with its 'sorta' inheritance. We managed to get rid of the custom constructor and have it rely on it's super class.

Finally, we noticed a nice clear division of labor in the complex method so we were able to extract a method that had a single responsibility. So we ended up with two methods that shared the complexity of the original one.

I would have been happy if all we accomplished was the last item. However, when you take a look at your most complex methods you often find bad stuff hiding amongst the craziness.

It's also important to note what we didn't do:
We did not simply extract out every 5 lines into a method. While that would have improved our Flog score per method, it would been the code equivalent of sweeping dirt under the rug. Methods should, as much as possible, have a single responsibility. If they don't then all you're doing is moving a bunch of complex stuff into many different arbitrary places, instead of just one place. Which is actually worse. So, like all metrics, complexity is something to pay attention to but you don't want to slavishly chase after good numbers.

Every time I need to do this I try to use svn's --help which is only possible to decipher if you already know what to do. Then I spend too much time on the internet looking for the answer. And finally I break down and ask my tech lead. With much shame. I'm putting this up here so I least I'll know where to find it. Maybe you too.

In the directory of the branch:

svn merge -r 1001:1002 https://svn.dev.your/repo/trunk/src .

If you checked in some files in revision 1002, then what you're saying is that you only want the changes from that checkin with '-r 1001:1002' Which is followed by the url of the trunk (where the changes were checked in) and a '.' to say that you want to merge to the current directory. Use '--dry-run' if you want to see what would happen without actually screwing anything up. After the merge is successful, you now have some modified files in the branch you can check in.

Done and done.