Pair Programming vs. Code Reviews

Jeff Atwood over at Coding Horror is asking for comments on the relative efficiency of these processes. We do both at my company, and while I don't think I have anything truly original to add, I come down on the agile side of the discussion (which should not be surprising to you if you read this blog often!)

The comments are already coming in complaining about pairing. I noticed these two particularly:

the obvious conclusion to this is double the hours per project, at minimum (and I'd expect that you would work slower if you had to discuss or explain stuff to someone else the whole day).

I would freak out if someone would watch me every the time I code (and also has a keyboard to interupt me lol)

Sort of the standard responses to pair programming. I'm not so experienced at the art that I can really say the hours don't double, maybe they do - but what I can say is even if the hours are doubling, the code quality is squared. Maybe it's just a commentary on what lousy code I produce by myself, but there is a big difference when someone else is there looking at the code, even if it's only the "navigator" effect, where the person who isn't actually at the keyboard can allocate the memory space to go back and remember any refactorings or other cleanup that needs to be done. As far as working slower, there are only two possibilities: first, that the other person doesn't know about the code as well as you do, in which case the knowledge transfer makes the whole thing worthwhile, or second, that there are a few ways of doing things and you need to decide which way is best. The selection you make when coding by yourself might easily not be that one.

even if the hours are doubling, the code quality is squared.

Freak out if someone watched you code? Dude, is your code really that bad?

Insofar as code reviews go, I find them almost unnecessary when pairing. Some teams do peer-review-before-checkin, which I don't really care for - I just can't grok the concept the code is trying to get across just from staring at it for a few seconds while someone explains it to me, but I suppose some people can do that. But we do code reviews for two things: first, to go over legacy code - we have plenty of that in our application - and second, to go over code that's just been checked in. This isn't 100% useful either, but on the other hand we have very few development meetings, and sometimes it's worth it just so someone can point out, "Oh, this should have been done using this brand new language feature" or, "we have a custom library that already handles exactly this case, can we use it here?"

So code reviews can be worthwhile, and they are absolutely necessary in a non-pairing environment. The big thing to watch out for is that you don't spend a lot of time discussing what your internal coding standards are, as I've written about before. But my feeling is that it is not as useful as pair programming.

iFrame scroll to anchor problem

So on my basketball fan page, HoosierBall, I was working on the schedule display. I had originally set up a simple widget from Zvents, but after using that for two years Zvents apparently changed their entire business model, from events created by users to events created by businesses. Every Hoosier game had already been entered via StubHub, the widget that I had been using no longer worked at all, and as far as I could tell there was no replacement.

But it's not like the schedule display needs to be real complicated. I tossed it into an iFrame, stuck the schedule on a separate page, and wrote some simple Javascript to scroll to a specific game's anchor based on the current date.

But what's this? When the iFrame scrolls, the entire page jumps down to the iFrame to display it. That's not what I wanted, but I couldn't for the life of me figure out a way to stop it from happening, until I finally ran across Jim Epler's blog entry explaining how he simply scrolled the main page back to the top after setting the anchor. So you set the location in the iFrame, the main page jumps down, then you set it back to the top. It's not pretty, but it works. Here's the code in the iFrame:


location.replace(location.href+anchorname);
parent.window.scrollTo(0,0);


Thanks, Jim!

Some test code smells

More of my writeup on the XUnit session I went to nearly two months ago. I hope someday to make all of my note-taking blog entries coherent to a larger audience :)

There are a couple of competing dynamics you get when writing tests: the first is that, in general, less code is better than more code. You want the code to express the concepts you need to express without any extra cruft, without huge globs of copy/pasted code here, there, and everywhere that is a nightmare to maintain. But you need tests as well, and tests are either more code, or more people, and people are one heck of a lot more expensive than code. So it's not at all unlikely that you would have as much test code as production code.

But those tests have to be maintained, so it behooves us to figure out the best way to do that, bearing in mind that the goals of test code are not the same as those of production code. So, here are some problems, or code smells, specific to test code that you might run into:

1. Conditional test logic . A lot of people like to say that one assertion per test is plenty. It seems like unreasonable test gold-plating to me, but if you're going so far as to put an if statement in the middle of the test, you need to have multiple tests to test each branch of the statement, each time. Or, the condition might simply be an assertion, where you're saying if (x) keep testing; else assert false. No point in that, just assert x at the beginning and let the code blow up if it needs to. This really helps with the readability of the test report, too, expecially if all the report says at the end is "The assertion FALSE occurred". Not helpful, whereas knowing directly from the report that X failed is much more useful.


2. Hardcoded test data

This problem is related to the principle that computer science profs have been tossing around since the beginning of time: that you never want to put numbers or strings in code; always define them somewhere as constants instead, so they can be easily changed. Not a bad principle; certainly it's ideal that anything that needs to be displayed to the user can be easily modified to use another language, so you don't want a bunch of MessageBox.Show( "Are you sure you want to do this?" ) statements scattered through the code. For numbers, though, my general rule is that it doesn't need to be a constant value unless it shows up more than once.

But in a sense, just about every number shows up more than once if written properly: at least once in the code, and at least once in the test for that code. Say for example you're testing a Price object with this line of code:

Assert.Equal( $14, CreatePrice().Retail );

CreatePrice() is part of your fixture, and it sets the list price to $20. Your Price object knocks off 30% to come up with the Retail number.

But now you've got the same number in there twice! See it? $14 is in there, and so is 70% of $20. The same number.

One fix is to move everything to constants. Presumably the Price object has a GetDiscount() method, so you could make the $20 into a constant ListPriceInDollars, and change the expected amount to ListPriceInDollars * GetDiscount. Still pretty verbose, but not really bad for this small example. A better solution can be to create an ExpectedObject to compare what comes back from CreatePrice to. In the ideal case, your test would then simplify to

Assert.Equal( ExpectedPrice, CreatePrice() );

Which would cover a boatload of other comparisons as well as your Retail value.

Request could not be submitted for background processing

Boy, don't you hate it when the request could not be submitted for background processing? I know I do.

This is a rather inexplicable error message that Crystal Reports pops up. If you search for the message, you see a few ideas for solutions, but what I eventually did didn't seem to be on anyone's list, so I'm adding this post to the search in case it's helpful.

In my case, the big clue was that we had just modified the report viewer so it could pick up data from alternate data sources based on information in the web.config. It's a pull report, so the data source is specified directly in the report and we're modifying it programatically. My assumption was that somewhere in the report, there was something being specified to use a different data source than all the rest of the report, and that our code that changed the source was missing it. But I couldn't find it, and Crystal report files have a binary format, so they're next to impossible to search through.

The big clue was that we had just modified the report viewer so it could pick up data from alternate data sources

I dug through a book on Crystal last night - luckily it had a chapter on dynamic data sources; thank you Brian Bischof - and one of the things it suggested was to check the connections on the tables using a member function, TestConnectivity. (You can outfit each individual table that a Crystal Report draws upon with its own login information.) To create a dynamic data source, you have to make sure that the source is changed all over the place in the report. In the report object, in any subreports that it uses, in any tables. So I set up the table login information to set it the way it was supposed to be, and then call TestConnectivity, and if that failed, throw an exception. My idea was that there was a single table somewhere causing all the problems, and that if I could figure out which one it was, I could fix it.

Only I didn't need to.

As soon as I added the TestConnectivity check, the report started coming up.

My working assumption is that the changes to the table login information are cached somewhere, and don't actually take hold until the report thinks it's necessary. Presumably there's a bug in the caching system, but calling TestConnectivity causes the table login information to be set properly.

Nice weird one there. Spent too many hours on it though.