The Difference Between Average and Really Good (again)
This morning I was pulling in code that had been checked in by one of the other developers in the group. Steve's code is, well... an interesting mix of optimistic code and a very obvious trial-and-error process. It's there, and it works, after a fashion, but even the Old Guard guy at The Shop called Steve's work "...slow and a little, you know, problematic." It's not that Steve isn't trying, he's working "hard enough" for The Shop, but had The Shop not had the "employment for life" plan, I'm guessing Steve would have been pushed out in one of the cuts.
Nice, but that's not really the issue - it's about making good, solid, reliable code in a short amount of time that is reasonably easy to maintain and has a minimal set of bugs that need to be fixed. It's about productivity, not popularity.
So today I knew I had to integrate some of Steve's code into the core of the web app I started and Steve's been adding pages to. I knew this because he asked me about it on Friday before I left - and I knew what to expect. I've had to do this a few times before.
When I got into the code, it was pretty much just what I expected - the basics were there, but a lot of the detail work was missing. For example, checking for nulls, putting in a new ivar's test in the Java equals() and hashCode() methods... things that aren't really killer bugs, but they could very well be a major pain in the rear trying to figure out if you didn't know that there were these weak points in the code.
So I fixed up all the problems so that the code looked like the rest of the code in the servlet, and helper class. I checked it in and sent Steve an email about the changes I'd made. What I didn't expect was Steve's response: mildly miffed.
You see, when I'd changed Steve's code for the helper class to check for the null on the new ivar, and changed:
public class UserInfo extends Object { ... private String[] _pages = new String[0]; ... }
to:
public class UserInfo extends Object { ... private String[] _pages = null; ... }
and then added the tests in the methods to check for null - as it was always possible to have the user set a null in the setter, I had broken his page. His page didn't check for nulls returned from methods, and Steve's point was that he didn't like making methods that returned nulls.
I can appreciate that he has a style, while I'm struggling to see why he'd want to make code that left out such a beautifully expressive value like null, I can see that he likes to write code a certain way. Good for him. But this isn't his code. It's not my code. It's The Shop's code. And because of that, we need to make it better than it needs to be for our private, individual use. It's got to work when we get bad inputs, when we get bad method outputs. It's got to just plain work. Period.
This is one of those seminal differences between average developers and good developers. The god ones know that they aren't writing it for themselves. They are writing code that's as good as they can make it. They know this doesn't take that much more time - but the effect is dramatic. It's the details.
Why would the return value from the servlet break the page? It should at least gracefully degrade capabilities. You don't want to have a page bomb just because the server died. Have a nice error message and then let the user see what they had last.
It's stuff like this that makes me wish I were in a group of a few, very hard working, very good, developers. Yes, it's a lot more work per person, but you don't have to worry about this kind of thing. You can divide the work and just know that the pieces will fit together and work well when it's all brought together.
What amazes me is that management knows this too, but they are seemingly happier with the idea that they'll be banking their success on the 'average' and just hope they can deal with things as they come up. Still kind of amazing.