Code Review for Git

I wanted to find a way to contribute to projects that I couldn't pair on, and to be able to scan a project for possible changes without sitting down and taking up someone's time. Sounded a lot like automated code review, so I had a look around at available code review tools. There are a few available - Review Board, Smart Bear and Crucible among them. I know Atlassian and generally like their products so I decided to install Crucible and give it a try.

There were a few confusing configuration problems. Until I read the documentation more closely (at all?) I wasn't aware that the beta Git plugin only worked against local git repositories, and I had to change a configuration file to convince Crucible that Ruby files should be treated as text instead of binaries, but once I got past these problems I clearly had a powerful tool for distributed, multi-party code reviews available to me.

So I created a review, looked at some code, and saw one thing that I would change. I wrote the comment, but things just felt wrong. It had taken me a while to type to comment (I'm not the world's fastest typist, but nor am I the slowest), and it felt like a waste of time. It would have been far faster to make the change and annotate the corresponding commit, but what I had was a code review tool, not an editor. Something wasn't working for me.

When I do a code review I want to achieve two things - I want to fix the code and I want to educate someone (where that someone may be me, if I wrote the original code). What I really wanted was something (an editor) that would let me focus on changes between versions, but let me change the latest version and save it. My VCS could store the comments against the (small) changes and I could point people to the VCS history to see what I'd done, potentially using the same tool.

After looking at a few different tools last night I've settled on Changes. Changes lets you compare directory structures and files, and it comes with some Ruby scripts that will help you retrieve specific revisions from Git, drop them into temp directories and review the differences. I've modified the script to use the current directory if you're comparing to HEAD, so that changes to the head are saved directly to disk and can be committed.

Here's my process:

1) Use GitX to determine the revision I want to use as the base
2) Use changes to view the differences
3) Incrementally work through the files either making changes or excluding the files.

I've created a (silent) video that shows me working through one review. I only make one small change, so it's mostly scrolling and excluding. One small point - the video uses an older version of my Ruby script that created a temp directory even if you were looking at HEAD, so changes were lost. I've fixed that in this version.

I saw some comments that Changes was rather expensive (US$49.99), but it's really cheap compared to many code review tools so I'm pretty happy with this approach.

Software Cultures

I think every programming language (and frameworks too) generates a dominant culture. It might be implicit in the language/tool itself, or it might happen more gradually through self selection. For example, here's how I distinguish three cultures (the boundaries are inherently fuzzy, and I'm aware that this is totally personal opinion, so people are bound to disagree by either raising counter-examples or saying they don't agree, and that's fine). I also have my tongue firmly in my cheek, but I believe there's an element of truth in each statement.

Java - a fascination with flexibility, generally achieved through complexity (and a healthy(?) does of XML). Ten years ago I would have said Java was focussed on solidity and reliability (in the meeting expectations sense), but I think this has changed over time. New functionality is implemented by adding new options. If it doesn't work, it's because you didn't configure it properly.

Rails - a fascination with bright, shiny new toys. Encourages the image of the developer as a person young enough to live in the code, who can deal with either new tools or tools that fundamentally change every three months or so. New functionality is achieved by implementing a new plugin/gem which needs to be compared to all existing alternatives. No solutions is 100% complete because people lose interest first. If it doesn't work, get in there and change the code yourself. [note - I don't comment on Ruby itself as I don't have enough exposure to the non-Rails community]

Seaside - I'm an old-hand in Smalltalk, just learning Seaside. So far I see a fascination with having something that just frackin' works (though it might be heavily engineered). The documentation sucks though, so get into the code. Changes are generally transparent and backwardly compatible. If it doesn't work, you did something wrong - the community is the documentation.

Each of these cultures has strengths and weaknesses. I'm 46 years old (shit, when did that happen), my raw memory isn't as strong as it was, and I don't think as quickly as I used to. But I have a load of experience in developing design models and applications - I want tools that get out of my way and let me build things. Given my Smalltalk experience, Seaside is a great match for me - much better than Rails. If there was the equivalent of "Agile Web Development Using Rails" then it would rock!

In an age when everyone needs to use portfolio theory to manage their software investments, starting low-cost initiatives by the dozen then abandoning the ones that don't work out, Rails is a clear winner over Java. It lets me experiment at low cost, it lets me get something out-the-door quickly. In that environment Java is a set of leg-irons. But the Rails culture thrives when people have enough time and energy to deal with a firehose of changes and alternatives - as an entrepreneur a lot of this is simply waste. One solution might be to standardise on a particular configuration - that seems like a false.optimisation.

What would you like in a code quality tool?

Although continuous integration has been a huge step forwards for the development community I think that over time we've tended to treat it as a rather large bucket that we could keep tossing things into. Sure, someone should immediately address that method that's too long, but should it really stop the build and the release of the next version if all the tests pass? My feeling is no, it shouldn't.

I think that the agile community could do more to encourage various parallel continuous processes that test different aspects of the application. In particular, we could separate the static analysis of the code quality from the behavioural analysis that we perform with tests, be they unit, integration or functional tests. If we separate the two aspects then we can look to improve them at different rates and in different dimensions. This separation is the motivation behind an idea that Marty Andrews and Simon Harris have been kicking around for a few years (Marty is the author of Roodi and Complexian, and Simon is the author of Simian, so they have a long-running interest in code quality). Cogent is now planning to turn this into a product - something that you can point towards your source code repository and simply unleash.

We don't have a name for the product yet (suggest one if you like), but it will run static analysis tools (like Checkstyle, Simian, Roodi, Reek, Flay) against each revision of your code base so that you can see trends over time, and it will produce warnings when the values of the metrics exceed certain thresholds. The product will be free for open source projects.

We'll start with a simple, web-based version, probably to run over Ruby projects in public Git repositories (because that's low-hanging fruit for us), but we're going to be very community driven. With this in mind we have a survey that will let you influence the development direction, even before we start. If a tool like this is something that interests you, please fill in our product survey. As a bonus, the first 500 people to complete the survey will get unlimited free access to the product in recognition of their contributions.

The cloud...it's full of *people*!

This is the story of the development of a small piece of software to deploy to the cloud, developed by a cloud of people, and how nominally unrelated groups interacted along the way.

I'm interested in having a way to easily create new Rails servers in the cloud (specifically AWS) and to deploy my apps to them. I'm familiar with the bits and pieces that are involved but I hadn't stitched them together. I started to write a Rails app to manage my AWS assets, partially as a path to learning extjs, but when the AWS Console came out I ditched that and started back on the original problem. Here are the different bits that I planned to use, and how I became aware of them:

• Sprinkle, by Marcus Crafter (http://github.com/crafterm/sprinkle/tree/master) - Sprinkle handles deployment of resources on Ubuntu boxes in a pretty easy-to-understand way. Unfortunately I have a strong preference for Postgres over MySql (blame Simon Harris) and the default Sprinkle deployment recipes all use MySql. Still, it didn't take me long to make some extensions to install Postgres instead of MySql.



• Passenger-Stack, by Ben Schwarz (http://github.com/benschwarz/passenger-stack/tree/master) - Sprinkles for Apache, Passenger, Mysql, Memcached & Git. I found out about this via a Ryan Allen tweet.



• Amazon Web Services (http://aws.amazon.com/) - well known, but the fairly recent addition of Elastic Block Storage (EBS) gives a way to easily create persistent data that outlives a given instance, say for a Postgres database.

The last piece to this puzzle is a web site called oDesk (http://www.odesk.com/). I was talking to a client last week and she mentioned that she thought Australian Rails developers were the most expensive in the world. When I asked her what she was doing she said that she was using off-shore Rails developers and was finding good people who would work for US$20/hr, from India but also from the USA. It seemed like the recession in the USA was having interesting side affects. I was definitely curious how using oDesk would work from a client perspective.

So I had this small problem that I didn't have time to work on, and my curiousity about oDesk, and it seemed natural to solve them together. I posted my job on oDesk - produce a script that, given an EC2 instance and an ESB volume, would automatically configure the instance to use Rails with Passenger and Postgres, storing the data on the EBS volume. In addition, provide a test application so I can verify the Capistrano deployment to the instance. I posted this as a fixed price job, and my plan was to use a few different people and compare the outcomes. I posted my job last Sunday (it's currently Thursday).

I had my first response in a few hours, from within Australia! Twenty four hours didn't bring any more responses, so I pushed my job in front of a number of Rails developers who had high ratings. Many declined as they were too busy, some thought the suggested price was too low given the need to learn about AWS, and some still haven't responded. I got two more positive responses, one from an experienced developer in St. Petersburg and one from a less experienced developer on the east coast of the USA. The bids were $333.33 (Australia), $250.00 (Russia), and $166.67 (USA). The expected development times were 1 day, 1 day and 1 week.

I accepted the responses and nominated a starting date of Tuesday. On Wednesday I got my solutions from Australia and St Petersburg. I've tested the Russian solution and it works fine. There seem to be couple of issues in the Australian solution but the developer
is being very responsive, and the USA solution isn't due yet.

What's more interesting is what else happened in the 24 hours of development. The Russian developer forked passenger-stack on GitHub and hosted the fork as a public repository on GitHub (which was fine by me). However, I found out about this through an email from Marcus Crafter, who knew I was looking for a Postgres solution and pointed the fork out to me! Then I saw these tweets from Ben Schwarz (@benschwarz) "@kouky Just so you know, someone forked passenger-stack overnight rolling postgres support in, I'll try and pull everything together :)" and "@atnan @kouky, I've just blindly merged the PostgreSQL stuff as I'm up in the hills. I'll check it all out tomorrow :)" The status message for passenger-stack on GitHub now says "Adding support for PostgreSQL. Sprinkle will prompt the user to select MySQL/PostgreSQL server and the relevant Ruby database drivers."

So it looks like my little experiment ended up sponsoring the change that I wanted in the core implementation of passenger-stack. I suppose I could have asked Ben to do it originally, but that seemed like an imposition and I would have only achieved one of my goals. Now I have a script to configure my EC2 instances automatically, a better feeling for oDesk and off-shore development, and I've discovered that you can always sponsor the features that you want in a piece of open source software. Well worth my costs :-)

Interesting behaviour for defined?

I've been working with Pete Yandell's Not-a-mock plugin, which I like a lot, and this morning a failing spec led me to some interesting behaviour for the Ruby defined? operator. Try these two things:

defined? arbitrary_attribute

defined? arbitrary_attribute=

With Ruby 1.8.5 on my Mac, the second one fails! From experimenting, it doesn't seem like you can use any setter. The workaround (which I haven't confirmed with Pete yet) is to use 'self.methods.include?("arbitrary_attribute-").

Template Methods in Java and Ruby

Template method is a pattern commonly used in Java frameworks to allow application specific configuration of a generic framework. Most of the behaviour is contained in a generic Base class (usually abstract), and the consumer provides their specific implementation in a concrete subclass that overrides methods invoked from the superclass template. Some form of dependency injection (often Spring) is used to let the application know what concrete class needs to be instantiated. Testing the concrete extension usually means testing that it invoked the responsibilities of the superclass as expected. Let's look at an example.

class Extension extends Base
{
void extensionMethod()
{
this.abstractBehaviour(someParameter);
}
}

I want to test interactions, so normally I'd use a mock, but I can't mock out the superclass. A brute force approach is to roll my own mocking by creating a new subclass for testing.

class TestExtension extends Extension
{
boolean wasInvoked = false;
void abstractBehaviour(Object parameter)
{
wasInvoked = true; // could be more complicated
}
}

public void testExtension()
{
TestExtension extension = new TestExtension();
extension.invokeFrameworkMethod();
assertTrue(extension.wasInvoked);
}

An alternative is to inject an object that encapsulates the behaviour as a strategy, like this:

class Extension extends Base
{
Extension(Implementation implementation)
{
this.implementation = implementation;
}

void extensionMethod()
{
this.implementation(this);
}
}

Then I can inject a mock or stub implementation and verify that it is invoked, and separately verify that the implementation does what I expect by injecting a mock/stub Base into the implementation. That certainly separates the two concerns, and each concern is neatly unit tested. It's also quite a bit of code. It also points us in a new direction for the framework - injection of strategy objects for the customisable pieces of code instead of using template method and inheritance.

Ruby can handle the problem using both these approaches, but there's a third alternative that isn't available in Java. In Ruby, I can directly change the behavious of the framework class.

class Base
def extensionMethod
abstractBehaviour(parameters)
end
end

All done. No new classes, no injection, and the entire application is aware of the change once the code is loaded. But how do you test it? We go one step further, and override the behaviour of just the Base instance that we are testing, like this:

specify "Should invoke abstract behaviour" do
base = Base.new
class << base
attr_reader :was_invoked
def abstractBehaviour(parameter)
was_invoked = true # could be more complicated
end
end
base.invokeFrameworkMethod
base.was_invoked.should == true
end

Of course, in Ruby you might still benefit from the flexibility of using injected strategy objects rather than a template method, but you might also be able to get away with something that is even easier than either of the Java alternatives.

Amazon links in Ruby

As I mentioned in an earlier post, I read a lot, and I want to be able to comment on the books I like, with links to a page about the book on my preferred book seller, Amazon. I also have an associates account with Amazon and I'd like to include that in the link, even though the last time I made any money from that was about 2001! Making the links has, frankly, been a pain the butt, but I finally dusted off my Ruby and used Amazon Web Services (AWS) to make this easier.

First I tried Ruby/Amazon, but this seemed to be using an old version of the AWS and I couldn't figure out how to do an ISBN based lookup, and I eventually abandoned it. In hindsight I should have done this earlier - the functionality I need was pretty easy to write directly in Ruby, and only the latest version of AWS seems to handle both 10 and 13 digit ISBNs correctly.

So here's my code:

require 'rubygems'
require 'hpricot'
require 'open-uri'

isbn = ARGV[0]

ACCESS_KEY = '01WXX7HHK8GBB3BFYX02'
ASSOCIATES_TAG = 'cogentconsult-20'

site = 'http://ecs.amazonaws.com/onca/xml?Service=AWSECommerceService' +
'&AWSAccessKeyId=' + ACCESS_KEY +
'&AssociateTag=' + ASSOCIATES_TAG +
'&Operation=ItemLookup' +
'&ResponseGroup=ItemAttributes,Images' +
'&IdType=ISBN' +
'&SearchIndex=Books' +
'&ItemId=' + isbn

doc = Hpricot(open(site))

author = doc.at("author").inner_html
title = doc.at("title").inner_html
detail_page = doc.at("detailpageurl").inner_html
image = doc.at("smallimage/url").inner_html

puts ''

html = "<a href='#{detail_page}'><img src='#{image}' alt='#{title}'></a><a href='#{detail_page}'>#{title}</a> by #{author}"

puts html
puts

Open-uri made the http access a piece of cake - definitely use this instead of Net::HTTP - and Hpricot was equally adept at giving me just the parts of the returned XML that I needed.

I can run this at the command line using "ruby booklink.rb someISBN" and I get the html for both an image link and a text link, that I can then paste into my web pages and edit ass appropriate. Hopefully I'll now be less reluctant to write about the books I've read.