There is a lot to like about code reviews, they extend knowledge, they allow developers with little experience in certain areas to work in those areas with the confidence that any dumb mistakes they make will be caught, and most importantly, they genuinely catch bugs - often very nasty subtle bugs at that.
At other shops I have done reviews of the 'cast your eyes over this before I press commit would ya' form, and these were low impact and high value. Here we are having a real quality drive in an (I think successful) attempt to pull a legacy product back from the brink of becoming a bug-ridden mess, and are doing long, involved, line-by-line dissections of every commit.
Here is what happens, like any department anywhere, some people are better at the job than others. Always fond of a massive generalisation I shall divide this entire gamut of human ability into two sorts of people which I shall refer to as senior devs and peons. Quickly, I find, the department falls into the state where peons do most of the work and the senior devs review it. Senior devs would love to do the work themselves, and would do it faster and better, and would use things like Patterns, and Modern Language Features, and all the wealth of experience and knowledge that has made them senior; but they are too busy with their reviews.
Ahha, you might say, but peons code slower, and so produce less work to review, so it should even out, right?
Would this were so, but peon work is often poor, or more often sort of okay but with subtle errors and missing some corner cases. It gets sent back, with constructive criticism carefully worded not to offend, and then returns, with a few different errors, and cycles backwards and forwards, and becomes a mix of intents, of the parts where the reviewer has put her foot down and explicitly dictated a design (sometimes I think I should just copy and paste a diff into the review comments) and, and here's the rub, parts where she has not had the heart, or the patience, to criticise any further, and has allowed the peon to muddle through in their own way.
I didn't bother picking up on the stinky bits in the first review you see, because at that point it didn't work at all and a bit of a pong was the least of its problems, and I kind of assumed they'd go when the thing was rewritten to, you know, work. But they did not, and now, on the third cycle of the whole depressing business, it seems cruel to pick up on them, and with heavy heart I am forced to let them through because, stinky though they may be, I'm just happy that they work.
And so you end up with code which, while it contains no known issues, is just not very good; it is fragile, and has bad encapsulation, and the tests are close to useless, and frankly, it sucks. Meanwhile it has actually taken longer than it would have taken the senior developer to do the damn thing themselves, and they would have done it better, and they are wondering if the department wouldn't be more productive without the peon in the first place.
No comments:
Post a Comment