www.codinghillbilly.com   kyle.baley.org  Subscribe / Contact
 
 
 
 
LATEST POSTS
Monday, September 06, 2010

Executive Summary: We’ve had to re-think some of the relationships between our objects with BigTable and, in some cases, reverse them.

One thing you can never accuse the hillbilly of is proper use of prepositions. Another thing of which you can never accuse the hillbilly is that he is not lazy. (Nor can you not accuse his use of double negatives of not being interesting.)

NHibernate has made me lazy about loading things. It’s also made me lazy about querying things. But in the BigTable world of AppEngine, I’ve had to actually think about both of these topics.

In our domain, a StaffMember has a collection of exactly seven DayAvailability objects, each one representing his or her availability on a given day of the week. I’d show you a nice little class diagram but I’m still trying to maintain a base level of laziness here. And this blog post is increasingly not helping.[lazy.gif]

Because it’s BigTable (which may be a NoSQL database if I had the gumption to look up what that exactly means rather than making assumptions based on the words involved), the collection of seven DayAvailability objects is stored with the staff member. I.e. I load the StaffMember, I get the DayAvailibility objects.

This is fine and dandy for our staff edit screen where we want to display the availability of the staff member. It’s also okay for our “add appointment” screen where we want to make sure that the appointment time falls within the staff’s availability.

Now let’s add a new scenario: A customer wants to book a massage at 2pm on Saturday. We open the book appointment screen and notice there are no slots available. The customer says “When’s the earliest after 2 I can get in?” We click on the handy new “find next available” link…

There are a couple of ways to implement this. Let’s try a naive approach where the system advances every half hour and checks to see if anyone is available that can perform the service. There are a few factors to consider but one of those factors is which staff members are available at 2:30.

In order to perform this query, we need to load each staff member and look through his or her availability. When it would be much easier to do a search like “Find an availability interval that includes 2:30 and return the staff member for it.”

Because of issues like these, we often have to rethink the relationship between objects. StaffMembers no longer carry a collection of DayAvailability objects. Rather, the DayAvailability object has a reference back to the staff member so that we can perform queries like the one above.

This affects the staff edit page because now we need an extra query. When we load the data for the page, we first get the StaffMember, then we query for all DayAvailability objects that refer to that StaffMember.

Basically, If you look closely, we’ve implemented a poor man’s lazy loading. The StaffMember now holds only the basic information. When we need its availability, we query for it.

After coming to terms with this, I decided it was actually a good thing. In many cases, we don’t need the staff member’s availability. So we don’t need to bandy it about like a wounded badger.

Who knew it was so much work being lazy?

Kyle the Re-edumacated

Saturday, September 04, 2010

Executive Summary: Introspection on how to cut corners for the initial launch of a public application with reference to Nate Kohari’s recent post on the subject.

As we amble forward on our little private venture, there are a number of applications and companies we watch closely. It’s hard to say whether we want to emulate them or if our philosophies naturally align and we’re attracted to like-minded people. Maybe a bit of both.

One of those applications/companies is AgileZen. From the measured approach to features to the deliberate way the user experience falls into the background, we not only use the product, we often look to it for inspiration. Sort of a “What would Nate and Niki do?” approach.

I read Nate’s account of the time before and after launch which a lot of nods in recognition. The almost universal message we’ve been given by people both successful and unsuccessful in building a public application is (and I’m quoting verbatim from one of them): “Just launch the &*%$ thing”.

This is what I thought about while I read the part in Nate’s tale about cutting corners. Initially, there was a lot of kludge and after launch, he rewrote almost every line of JavaScript.

He makes no apologies for this nor should he. It is, I think, a valid decision for a product like his (and ours). Provided you’re smart about it. Recognize where the crap is and isolate it and make a concerted effort to go back to it after you have money coming in. The point he makes is:

Before we could afford to spend a lot of time to make the software maintainable, we first had to prove that we would actually have to maintain it – we had to make sure that we actually had a market.

It’s something I’ve wrestled with a little, especially as we had a learning curve of using an unfamiliar technology. How far do we cut corners? Forego UI tests? Unit tests? Proper separation of concerns?

My partner is a little more battle-scarred in this area and he was pretty adamant that we keep this reasonably maintainable from the start, even if it takes longer to write code, which it inevitable would. Which means we would have to sacrifice something on the iron triangle (or the lesser known Pewter Scale). But that was an easy choice. We wanted relatively high quality and to deliver on time and we were constrained by budget. That left scope as our only real variable. And I love me some restricted scope. Features are, to my mind, the only aspect of the triangle worth arguing over.

But back to the quality aspect. I’m speculating but I suspect we didn’t cut as many corners as Nate did. Or maybe we just didn’t cut the same corners. Though I won’t claim anywhere near 100% code coverage, we have a great many unit tests. And as part of our process, no story can be marked complete unless it’s been code reviewed by at least one person. What’s missing so far is a formal QA person, which is a matter of not having the resources (or put another way, not placing a high enough priority on it based on the resources available to us), and a UI testing process, which is near the end of prototyping thanks to Winnipeg’s best kept secret.

So the question is, did we hurt ourselves but placing an undue focus on quality aspects of the process we didn’t need to?

I have no idea. And while it makes a nice, lengthy blog post, in the end, I haven’t been stressing about it much (at least not until I read the blog post; thanks for nothin’, Kohari). But here’s the position we’re in: We’ll be releasing beta next week and our first order of business is to start looking at new features, which we will release at regular intervals (at least bi-weekly, if not more often). We have no refactoring to do because our codebase is exactly where we want it to be architecturally.

The difference is we’ve lost a month-ish worth of income. And at the moment, that income is still a big unknown. AgileZen could have launched to a whole lot of nothing in which case, Nate would have chalked it up to experience and thanked his lucky stars he didn’t invest any more time than he did in it. We may do the same and realize that we should have launched sooner so we hadn’t sunk any more effort into it.

There’s an element of faith involved though. Maybe it won’t catch on but I really believe it will. And if I plan to be looking at this code for the next five to ten years or more, I like the fact that we put a little extra effort into it early on.

Another nice side effect of this is that it’s helped defined the culture of our development team to a large degree. It helped us to build the team we have and by setting the ground rules early, it’s helped us maintain a high standard so that it’s almost become second nature by now.

Man, this blog is saving me a ton of money in psychologist fees.

Kyle the Clear-headed

Wednesday, August 18, 2010

Still neck-deep in start-up mode these days. I still don’t feel any different than I did before we started this venture whole hog in January but that might be because I was prepared for it. Since the baby came in November, I was already familiar with the lack of sleep, persistent anxiety, and the constant mood swings between “this is the greatest feeling ever” and “what the &*%$ was I thinking?!?” so it was a smooth segue to entrepreneurship.

In any case, I’ve waxed somewhat comprehensible on the topic of working remotely fairly often. In most cases, I worked on a team where I was the only remote person. Nowadays, I’m part of a start-up where the entire dev team is scattered. And long-time readers will be horrified to hear that I’m the one in charge.

It’s been a learning process, to be sure, and continues to be. We’ve adopted procedures that I increasingly refer to as “spry” in that they help us work faster. But I’m scared to call it Agile or Lean because my knowledge of either of those processes is limited to what I read about it in the little-known Bazooka Joe comic series on the subject. So in order to avoid much “tut-tut”-ing from people on Twitter, I just say “this is what we do; if it follows a formal methodology, that’s not my fault.”

To that end, I’m going to fly directly in the face of the “favour individuals and interaction over processes and tools” part of Agile and post on a few of the tools we’ve been using to keep us organized. Or, more accurately, keep us from becoming too disorganized. In this episode:

AgileZen

When we first started our venture, my partner had everything in BaseCamp. It worked well enough. The Writeboard feature was useful and it delivers on its promise of keeping communication centralized. But we were using a task list to manage the features we wanted to put in and it very quickly became unwieldy for that. Not to knock BaseCamp. It is in the realm of software we want to emulate in that it’s great at what it does and makes no apologies if it doesn’t work outside of those boundaries.

The two obvious choices in my mind were LeanKitKanban and AgileZen. We took a look at both tools and decided on AgileZen because of its simplicity. We also took a very brief look at AxoSoft but it was deemed overkill for our needs. The pricing page alone doesn’t exactly inspire confidence in the whole “keep it simple” thing we were trying to strive for.

Likes

There’s plenty to like about AgileZen. The visual nature of it is well-suited for a remote team for daily stand-ups. Our process is generally to leave the board as-is for most of the day and move things around just at the stand-up. Helps give a sense of progress that was accomplished the previous day.

AgileZen follows a growing trend with software: less is more. This is an approach espoused by 37Signals and it’s one we’re very much on board with in our own product. So it gives us a nice warm fuzzy feeling to use a product that aligns with our own philosophy.

The overall user experience shows great attention to detail. It wasn’t until very recently, after having used the product for months, that I noticed the Add Story “form” had no labels. Instead, all the fields have watermarks indicating what to put in the field.

Because of this, the overall feature set doesn’t really give a decent picture of the application. Yes, you can colour code stories and add tags and, more recently, filter based on search criteria. None of these individually make AgileZen better than competitors. It’s more the way they all seem to work as a cohesive product.

Wish List

There is but one single feature that I think is the sole major gap in the product: a full API. There is a REST API to query things but nothing to write information back. (At least not yet; the feature is in the works.) This is something that would have *really* come in handy when we migrated all our tasks out of BaseCamp. But even now, stories come from other sources and almost all of the other products we use (e.g. BitBucket, ZenDesk) have an API so it would be pretty trivial to write a little utility to, say, take a ticket from ZenDesk and create a story from it in AgileZen. (And as I noted on Twitter, no prizes for what such a utility would be called.)

Also, as much as I marvel at the user experience, there are some minor quirks. The drag and drop isn’t infallible. There have been many cases when I just wish there was an arrow icon that I could click to move it to the next column without me having to fight with drag-n-drop, especially on my laptop where I’m forced to use the touchpad.

Couple of other annoyances are very likely low priority due to the potential number of people it affects. First, I use the Vimium Chrome extension which makes it easy to navigate a page with the keyboard. But some areas of the app aren’t “clickable” with this extension. Second, the fact that it kicks me out after I log in from another computer. I use AgileZen from at least two different virtual machines on any given day so I often have to re-login. This has been alleviated thanks to Vimium and AutoHotkey using a similar technique I used to migrate the stories out of Basecamp. Now, I just press Win+a to log in to AgileZen.

I hesitate to mention this possible feature but it’s something that would come in handy for us: threaded discussions attached to a story. The reason I hesitate is because, frankly, if I were on the AgileZen team and someone asked for this, my answer would be a flamboyant “suck it, user!”. It pretty much flies in the face of everything AgileZen is trying to achieve with its feature set. Internally, when we discuss a feature like this that would very obviously make the app overly complicated, we usually say discard the idea and say, “Yeah, there’s no need to Microsoft the app.” That’s how I predict the conversation would go with threaded discussions in AgileZen.

This list is kind of a double-edged sword because as I mentioned, any new features threatens the “keep it simple” vibe the app has at the moment. That said, Nate and Niki are smart people and I trust they will find a way to give me what I need, even if it’s not exactly what I asked for.

Wrap-up

Besides using the software, AgileZen has a certain quality that we are actively aspiring to achieve. It has the type of user experience that produces an emotional response. That response is almost universally positive so much so that even though it has a few quirks, I love using it. It’s something we use everyday and coupled with a decent screen-sharing tool (which is coming up next), it’s been a valuable tool for our remote team.

Friday, July 30, 2010

Executive Summary: Use a formula in an NHibernate mapping to facilitate searching the entire string, “LastName, FirstName”, for a User object.

Will see how long this Executive Summary thing lasts. Getting tired of people wasting my time by posting comments saying I’m wasting their time. (I’m also working on an idea for curbing “Smells like fail” comments as well but it’ll involve some serious changes to your browser. Or, based on the average age of people that say things like that, a call to your parents to discuss how much time you spend on Facebook.)

When I’m not Google Web Toolkittin’, I have a nice side project that I use to keep my .NET skills sharp, keep one foot in the door, and whatever other reason I can think of to avoid saying the real reason, which is “pay the bills”. Because one thing start-ups ain’t got a lot of is stable (read: any) income.

In said project, I have a page with an auto-suggest feature to search for users. I.e. you enter some text, and it finds any users with the entered text in the name and displays them in a dropdown. I’d show a screenshot but in the time between when I developed it and when I wrote this, the feature was dropped.

The mechanics of the auto-suggest might be the subject of another post but I doubt it because it’s been covered to death (though not so much in ExtJS which is what we’re using). I’m going to talk about what happens in the back-end. That is, how do I get the data from the database with NHibernate.

We’re using Linq to NHibernate so my first pass was straight-forward:

public IList<User> Search(string searchText) {
    var session = NHibernateSession.Current;

    return ( from w in session.Linq<User>()
                where w.FirstName.Contains(searchText) || w.LastName.Contains(searchText)
                select w).ToList();
}

This works exactly as one would expect. If the user enters "will", it will display "William F. Buckley" and "Ted Williams" and "William 'Wild Bill' W. Williamson" in the search results. Or rather, it will show "Buckley, William F.", "Williams, Ted", and "Williamson, William 'Wild Bill' W." because that's how we're displaying our search results.

And to facilitate that display, we have a Name property on the User object:

public string Name {
   get { return LastName + ", " + FirstName; }
}

Problem is that this search doesn't cover a common scenario. What if the user types 'Williams, T'? This would be a natural thing to do. They want Ted Williams, so they start typing Williams. The search results are too big and they are showing items in the "Last, First" format so it makes sense to keep typing and try to narrow it down further.

The code above will return zero results for such a search. Really what we want is to search the Name property, like so:

public IList<User> Search(string searchText) {
    var session = NHibernateSession.Current;

    return ( from w in session.Linq<User>()
                where w.Name.Contains(searchText)
                select w).ToList();
}

Which doesn't work either because Name isn't a database field and as yet, NHibernate is not able to parse formulas in your properties and convert them into SQL or Criterion.

But NHibernate *does* allow formulas if you describe the formula to it in the mapping. We're using Fluent NHibernate (assuming it hasn't been merged into the NHibernate project yet and completely replaced mapping files, which it should be):

public class UserMapOverride : IAutoMappingOverride<User>
{
    public void Override(AutoMapping<User> mapping)
    {
        mapping.Map(x => x.Name).Formula("LastName + ', ' + FirstName");
    }
}
And update the Name property in the User object accordingly:
public virtual string Name { get; private set; }

Now, our Search function works the way I want.

Kyle the Formulaic

Friday, July 30, 2010

Bug tracking probably means something different to you than it does to the Hillbilly but if it gets you half as excited as it gets me, be sure to wander on over to http://youtrack.codebetter.com. As a sister-aunt service to http://teamcity.codebetter.com, we’re now offering a free issue tracking service to OSS projects, courtesy of the gentle folk at JetBrains.

Hadi Hariri has more details on how to sign up. Despite doing all the work, he also graciously thanks me for all my hard work and if replying to emails with “Good idea” and “Let’s do it!” and “If you think I’m supporting all these &*%$ whiners and their ‘issues’ then you’d better add ‘Hadi’s Personality’ to the project list ‘cause you got issues of your own” is all it takes to get gratitude these days, then I’m owed a whole lotta appreciation.

Happy bugging!

Kyle the Etymologist*

*Yes, yes, I know. I’m trying to be subtle here.

Wednesday, July 28, 2010

Executive Summary: With all of our experience in Microsoft technologies, why did we choose Google Web Toolkit over other technologies for our start-up?

Almost six months ago, I described my initial reaction to Java coming from ten years in the Microsoft world. At the time, I tastefully glossed over the reason we ended up using Java, or more specifically, Google Web Toolkit saving the discussion for a time when all I really wanted to do was make sure there were no gaps in the list of months in my Archive. Plus the original post inspired would-be blog-reviewer, Dinesh Gajjar, to comment on the lack of content, a position that leads me to believe he’d only recently started reading my blog. In any case, one of his concerns was that I didn’t explain “why I chose what” which I will assume means “why I chose GWT” (and apologies, Dinesh, if by “what”, you really meant “to waste time that could be spent writing comments on other people’s blogs”).

When first deciding on how to proceed, our default was .NET, which led to some obvious options. MVC seemed a no-brainer and since I was involved in Sharp Architecture, my first reaction before we even finished the conversation was to run the Sharp project template. “But,” says we, “we’re pragmatic programmers. Why do we automatically reach for .NET? This is *our* project!”

Thus explains why my resume says I have two days of experience with Ruby. I played with it for (almost) that long before we changed our minds again. As we discussed the nature of the application, we discovered it would rely *very* heavily on JavaScript. So we wanted something that would facilitate that.

At the time, we were also a little apprehensive about the learning curve with Ruby. Not that we wanted to shy away from it. But this wasn’t some fictitious client’s money we were playing with. It was hours. “Learning on the client’s dime” had a more ominous effect when the client’s dime was my dime.

So we ventured back to .NET being familiar territory and telling ourselves “it’ll help us get this out the door faster.” We also looked seriously at OpenRasta because it seemed to facilitate our view of having the server serve up resources rather than the default view of serving up pages. Our intent was to have a single page and dynamically modify the various pieces view JavaScript. We weren’t looking forward to managing history support for something like that but we felt it offered the best user experience, which is more important than our nagging little technical problems.

One nagging aspect to this approach was testing the JavaScript, something I had little experience with. I did some smoke tests with WatiN and was encouraged a little but it always felt a little uncomfortable, especially trying to run the tests on a CI server. And both my partner and I (admittedly, he more than I) were concerned about the costs of long-term maintainability and were pretty adamant on making the app testable and following a strong focus on quality while we built it. WatiN didn’t give us that warm fuzzy feeling and I don’t believe we looked at Selenium at the time.

Around this time, my partner discovered Google Web Toolkit which looked tailor-made for the type of application we wanted to build: A single page with various pieces updated based on user clicks. And you could write it all in Java, which was easily testable. And better yet, it had history support built in.

So we both did smoke tests and decided it was the way to go. To summarize, it gave us (in order of importance):

  • an AJAX application with a good user experience
  • history support (also important for user experience)
  • easily tested
  • strong community and many resources

In retrospect, it seems there is a fine line between “pragmatic” and “indecisive”. There’s a good chance that not all of our decisions were the right ones, just the ones we made at the time with the information available to us. As I reflect on it, maybe we dismissed Ruby too soon. Or maybe we could have built the app faster in .NET and still made it maintainable. Or maybe or maybe or maybe…

Truth be told, writing this post is about all the reflecting I’ve done on it. Because I have no regrets using GWT whatsoever. It’s lived up to its promise. Yes, we’ve run into issues but none that have been insurmountable and certainly no more than we would with .NET (can’t speak for Ruby).

More importantly, it’s because we use Java/GWT that we have the team that we do and, with all due respect to the awesome team I worked with on my first Livelink project a couple of years ago, it is by far the best team I’ve been in in my twelve years of consulting. And this from a guy who gets along with everyone (almost).

Kyle the Indefinite

Wednesday, June 23, 2010

Quick PSA today on automating tasks in web apps that don’t have keyboard shortcuts.

I had to migrate about 110 stories from one AgileZen project to another. As yet, AgileZen’s API is read-only so after confirming that the eldest offspring was unwilling and the youngest (being seven months old) was unable, I set up doing the task manually.

(Side note: The reason for the migration is that we were originally using BaseCamp. All our stories were stored as tasks for our beta version and version 1. We had originally moved all the V1 stories into a fake phase for AgileZen just for the sake of getting them out of BaseCamp. At the time, I glanced through the AgileZen API documentation and had mistakenly concluded that I’d be able to migrate them to a new project programmatically when the time came.)

Copying the stories wasn’t too onerous a task. They were all in the same phase and didn’t have any details assigned to them so it was about fifteen minutes of cut and paste from one browser to another.

Deleting the original stories proved a little more monotonous. They can only be deleted by opening the details, clicking Delete, then clicking OK. After doing this manually about five times, I started exploring other options.

On Chrome, I have Vimium installed. It gives you Vim navigation keyboard shortcuts, similar to Vimperator for Firefox. With it, when you can “click” links by pressing f, then typing the appropriate code corresponding to the link. That means I can handle the clicking involved to delete a story from the keyboard.

I also have AutoHotKey, which is designed to automate keystrokes and mouse clicks. I use it to create keyboard shortcuts like Win+n for notepad and Win+g to open the hillbilly community forums website and an alternate way to close an app without using Alt-F4.

Between the two of them, I came up with this quick ‘n dirty script to delete stories:

#q::
Delete()
return

Delete() {
  Send f
  Sleep 250
  Send s
  send h
  Sleep 1000
  Send f
  Sleep 250
  Send a
  Send f
  Sleep 500
  Send f
}

Now, when I press Win+q, the following happens:

  • Press f to tell Vimium I’m going to navigate to a link
  • Pause a quarter of a second; without this, the script goes too fast for Vimium
  • Enter the key code for the story I want to delete. Note that this is always the top story in the phase so the code is always the same as long as there is at least one story in the phase
  • Pause a second to give the browser time to navigate to the story
  • Press f again for Vimium navigation and pause
  • Enter the key code for the delete button
  • Pause half a second to give the confirmation dialogue time to appear
  • Press f again for Vimium navigation

At this point, I enter the key code for the OK button manually just to be on the safe side.

Pausing for a second to let the browser navigate to the story details is a minor hack. I’m pretty sure I could get AutoHotKey to wait until the title of the window changes or something else more accurate. But I have to balance out the time it takes to figure this out versus the time it takes to do it manually…ignoring the time it took to write this post, of course.

Kyle the Served

Tuesday, June 22, 2010

I have a new mistress and her name is Rietveld. I call her Veldy or sometimes, in the throes of passion, RV. But only because I have no idea how to pronounce Rietveld out loud.

In a recent post, I talked about our switch to the code review tool. And by recent, I mean less than a month so it’s been a torrid affair but I can tell she’s the code review tool I’ve been waiting my whole career for.

In that post, I described our old process and the proposed new one. Now, a few weeks later, I can begin my mission to make sure every man, woman, and child knows about the tool whether they do code reviews or not.

Our process is the same as outlined in the previous post but I’ll mention some side notes here just to ensure there’s new content in an attempt to keep potential trolls at bay.

The absolute greatest feature by far of the tool is how it enables asynchronous and disconnected reviews. By “disconnected”, I mean I don’t need to have the code with me. (You still need an Internet connection.) I just finished a ten day trip to London* and performed at least half a dozen reviews during that time on my wife’s laptop. Everything I need is included in the review page: the old code, the new code, and any comments by other reviewers or the initiator.

There are a couple of things this has enabled. First, I can do code reviews any time I have a free moment. I whipped off at least four this morning. Before I hit the sack for the evening, I’ll often open Rietveld up and bang out one or two just to end on a high note. It’s rare that I spend more than half an hour on any of them. More often, it’s closer to fifteen or twenty minutes.

If you’re wondering why we have so many reviews, that’s the second “enabled thing”: It’s so easy to do them, we can now review everything. We even have our own process swimlane for stories that are to be code reviewed. I.e. a story can’t be marked done until it’s passed the review. Ditto for bugs. The reason this works? Because it’s trivial to start a review and it’s almost trivial to do them. At this given moment, we have fifteen active code reviews. I expect at least three of them to be closed in the next twelve hours. But I’m also expecting two more to be created in that same time.

And because it’s asynchronous, I don’t even need to finish a review when I start it. Any comments I’ve made that session are stored as drafts until I publish them. I’ve done this a couple of times: Hit some code that required more thought than I was able to give at the moment so I just shut down the review and moved on to the next one.

There is a nice side effect that’s come from this process. Our team consists of one junior developer, a senior developer, and me. Because the comments are persisted and because we don’t allow stories to continue until the reviews are complete, the reviews have become a good source of training and mentoring for the junior dev. She will submit a review for a story, launch a review, and then update the code several times addressing issues that were brought up during the review. Sometimes the comments are trivial, like “make sure the indentation is consistent”, sometimes it involves a fairly sizeable chunk of work, like “add unit tests to expose the bug”. During this time, she can submit further patch sets and Rietveld can show you the diffs between them and the previous patch set. All the while, we (and by “we”, I almost always mean “the senior dev”) are providing on-the-job mentoring that’s relevant and contextual.

“Code review” no longer holds a connotation of six people in a room for hours being kept conscious by whatever substance passes for coffee these days. Now, it’s become a relatively benign task, like verifying we have unit tests. Story’s done; launch a review and move on to the next one.

So if you meet the requirements (use SVN, Hg, or Git; can create an app on AppEngine), you’d make an old-ish Hillbilly mighty happy iffen you’d check it out.

Kyle the Renewed

Wednesday, June 02, 2010

For me, code reviews have traditionally been one of those aspects of software development that fall into the “good idea; bad execution” category. It’s something that I know we’re supposed to do but when someone says, “let’s do a code review”, my first reaction is usually to make a cross with my fingers and start yelling “THE POWER OF CHRIST COMPELS YOU!” at them.

Unfortunately, our team is scattered across three time zones these days so that tactic is less effective than it has been in the past. Which means we’ve had to address the real problem: how do we make code reviews useful?

Our first attempt was to do them the tried-and-almost-true way. That is, we scheduled a code review meeting on Wednesdays and I’d send out notices to the other developers on Monday saying we’re reviewing these five classes. I sent the notes out on Monday to give people plenty of time because it wouldn’t be realistic to expect them to do the reviews an hour before the meeting starts now, would it…

Historically, code review meetings that I’ve attended/led have usually followed a familiar pattern. To summarize:

Meeting starts
Coding Hillbilly: Okay folks, I’m pumped to get started. There are some interesting things going on in this code and some awesome comments. Let’s take an easy one to start off: “This code is pretty coupled to the implementation. We should create an event bus to manage the communication between the various presenters.” I agree and let me tell you at length why I say that…

End of Hour One
CH: I’m going to have to cut off your overview of dependency injection frameworks, Arbuckle. You’re right, I think that will help us with the internationalization of our config files. But we’ve only covered three issues so far. The rest of them should go much quicker though. Let’s take a look at the next comment…”This should be split out into a REST service”…

End of Hour Two
CH: Moving on…”We shouldn’t need a separate interface for this. It’s just extra complexity. Just create a new instance of the implementation in the constructor.”………<sigh>…Agreed. Next…

End of Hour Three
CH: Next…”Move the hard-coded database connection string out of the vie---“…you know what? The rest of this code looks good to me. Any objections if we accept it as-is? Great. See you all next week.

In this little slice of life, I’ve also ignored an important aspect of code reviews with remote teams. In an attempt to keep comments as close to the code as possible, they are done as //TODOs inline with the code. And to avoid annoying merge issues, this means everyone needs to do the actual reviewing at different times. So there’s a lot of “Okay, I’m done. Have at ‘er” IMs thrown about.

As much fun as this is, I decided to give Rietveld a look. I stumbled across it when I noticed Philippe Beaudoin, the creator of gwt-platform, was using it for his project. It was easy to pick his brain about it because, y’know, he’s on our dev team.

Some notable tidbits about Rietveld. It’s based on Mondrian, the internal code review tool used by Google. And it was created by Guido van Rossum. As a general rule, if the creator of a piece of software has also written a widely used programming language, it’s worth a quick eval.

But it was the workflow of Rietveld that drew me in. This is our new code review process:

  • A developer initiates a code review by selecting a starting revision and an ending revision. The diff between the two is submitted for review to the selected reviewers
  • The reviewers look at the diffs (either as a side-by-side diff or, if you prefer a challenge, a unified one) and draft comments inline with the code on the web app (i.e. not in the code in the repository itself)
  • When a reviewer is done drafting comments, she publishes them to other reviewers and the initiator
  • The initiator reviews the comments, makes changes if necessary, and publishes updated code diffs in response to the comments (again, if necessary)
  • The initiator and other reviewers can keep adding comments back and forth. The comments are stored in a discussion format similar to how they appear in GMail.
  • Once all comments have been addressed (either through code or discussion), the code review is marked closed.

Here are the parts I love about this:

The reviewed code is a diff

We don’t need to review an entire class every time. We just look at what’s changed from one revision to the next. So we can focus on the functionality for a specific bug or user story. This makes it easier to implement a policy like “User stories must be code reviewed before they can be marked complete.”

At the beginning, this will be a pretty moot argument since diffs will include a lot of new classes. But even still, we can see *all* the changes that have been made for a particular story, including that “minor” change you made to the GetUsersTimeZone function in the global DateUtils class.

Review comments are an ongoing discussion

The comments are threaded in the interface and capture more of the discussion than our previous process, where one developer would add a //TODO and the rest of the discussion was done at a meeting. And speaking of the code review meeting…

NO CODE REVIEW MEETING!

This is by far, my favorite aspect of Rietveld. Once launched, code reviews are done asynchronously. Reviewers add comments whenever it is convenient (within a reasonable timeframe, of course). Other reviewers can chime in when they want. Once published, comments are immediately seen by other reviewers and can be commented upon further. For a disparate team like ours, this feature alone is pure 100 proof homemade moonshine. And finally:

The code review isn’t done after the meeting

In our previous process, the code review was all but forgotten after the meeting. There was no inherent follow-up process to make sure the changes had been made. Here, the code review remains active until it is explicitly closed. I.e. once all comments have been addressed to the team’s satisfaction.

sally-code-review.pngNow to implementation details. Rietveld is a Django app running on Google App Engine. There’s a publicly hosted implementation of it at http://codereview.appspot.com. There, you’ll find a truckload of OSS apps registered for it (and in fact, Rietveld is open source itself if you feel like looking at Python code written by the creator of Python). Nice thing about that is that you can see examples of code reviews in progress.

If, like us, you are leery about publishing your reviews publicly, you have at least two options that I know of to host it yourself. One is to create an application for yourself on App Engine and host it. There are instructions to do that and I’m sure they work. We didn’t try them because the other option is much easier. If you have a Google Apps For Your Domain account, you can add it directly from the Google Apps Marketplace. One click and it’s added to your account and only you and your domain users have access to it.

From what I’ve read, Rietveld works with Subversion, Mercurial, and Git repositories. From experience, they can be private repositories as well as public ones.

It’s probably too early in the process to be extolling the virtues of our new code review process. At this stage, Rietveld is kind of a rebound girlfriend for us because of the pain we felt with the old process. But early indications are that it will be a big hit.

If not, I suppose we could print out the code and fax comments back and forth…

Kyle the Encoded

Disclaimer
The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.

Copyright © 2010 Kyle Baley. All rights reserved.
 
CATEGORIES
.NET General (18) alt.net (4) altnetconf (9) ASP.NET AJAX (40) ASP.NET MVC (29) Bahamas (1) Bahanet (9) BDD (1) Brownfield (21) Career (10) Castle (1) Code coverage (1) Code review (2) Coding Style (6) Communication (1) Community (18) Conscientious Coding (36) Continuous Integration (11) dasBlog (12) Development (16) DevTeach (4) Domain (2) Environment (4) Estimating (1) Featured (14) Flamingo (10) Games (1) Google App Engine (4) GWT (9) Hardware (6) Java (2) Javascript (7) Linq (3) Livelink (6) Lucene.NET (2) MbUnit (1) Metrics (2) Miscellaneous (25) Mocking (4) NAnt (4) NHibernate (13) NInject (1) Office (3) Office Development (6) Open Rasta (1) Patterns (6) Presenting (14) Professional Development (15) Refactoring (10) ReSharper (11) REST (3) S#arp Architecture (5) Security (3) Software (11) Sundry (19) TDD (19) Tools (23) User Interface (6) Utilities (9) Visual Studio (8) VSTO (1) Web development (12) Windows (3) Working Remotely (18) Workplace (3) Writing (6)
 
LATEST POSTS
 
POPULAR POSTS
 
 
ARCHIVE