TechBookReport logo



Meet Joe Bloggs

Episode 24: Code Review

I hate it when the Boss starts talking about best practices in software engineering. He thinks it makes him sound like he knows what he's talking about. We think it makes him sound like he's been bought lunch by a software vendor or he wandered onto a techie web site whilst looking for smut. This time he's decided that code reviews are exactly what we need. I explain patiently that code reviews were tried by the Boss before him and failed, but he's not convinced. Even when I explain that the Boss before him was carried out of the building by paramedics he's adamant.

So, here I am, sitting in an unfamiliar office, surrounded by unfamiliar people, working on unfamiliar projects. I hate having to come upstairs. I'm sitting with Derek, a project leader, and Megan, one of his team. They look as happy as I do. I've been here for five minutes and so far the we've discussed the possibility of rain six times and the state of the roads four times.

The Boss arrives finally. A big hearty grin on his face. Yep, it has the desired effect of instantly bonding us as a team.

'This should definitely not be a confrontational meeting,' he announces. 'This is about swapping experiences? learning from each other?fruitful cross-pollination…'

'I thought it was about looking at code,' Megan interrupts two minutes later.

'It is, it is,' the Boss assures her. 'But in the process of sharing code we can learn from each other, pass on the benefits of previous experience. It's about process improvement. It's about…'

'…time we started,' I suggest.

The Boss looks deflated. I guess there's at least another ten minutes of verbiage that he's got lined up.

'Agreed,' Derek adds helpfully. 'We've only got this room booked for another forty-five minutes.'

'Yes, good point. So, let's just dive in,' the Boss says excitedly.

Megan, Derek and I all turn towards him and wait.

'Aren't we starting?'

'We are,' I point out.

'You don't think I should sit in?' he asks hopefully.

Vigorous shaking of heads on our side. 'To be honest,' I tell him gently, 'it's likely to be a bit technical.'

'It's Java code,' Derek adds.

'Twenty pages of it,' Megan chimes in.

And then I deliver the killer blow. 'There are no pictures.'

The Boss looks frightened now. 'Not even a diagram?'

'No,' I tell him. 'We decided that PowerPoint was banned from code reviews. The code needs to speak for itself.'

Nods all round from our side of the table.

'Perhaps I'll give this one a miss,' the Boss finally concedes. 'My Java's a bit rusty to tell the truth.'

'Well,' I tell the others once the Boss has gone, 'if we ever need to do a code review on a three line DOS batch file we'll know who to call.'


Twenty minutes later I'm knee deep in code and struggling. Derek's decided that he doesn't need to sit through another boring meeting, and once Megan is happy that I'm not about to knife her in the back he disappears. Megan explains that the code I'm looking at was part of a maintenance release on an existing product. It's part of the start-up code but the complexity of it is way over the top.

'So,' I say, 'let me get this right. You've got a splash screen and a progress bar and this bit of code is spawned to a separate thread to grab the environment details. Right?'

Megan nods.

'And there's a settings file which contains everything you need. Right?'

Another nod. 'It's an XML file stored in the user's settings directory.'

'So you grab the file, check that it's well-formed, validate against a schema and then transform the contents into text.'

'We store the XSLT file with the program code.'

'And you check that for well formedness and validity as well I see.'

'Can't be too careful,' she says.

'And then, once you've got the transformed text you read a set of simple properties and feed these back to the rest of the application.'

'Yep.'

'The code's spot on,' I remark.

Megan blushes. 'Well, I admit I did clean it up a bit yesterday.'

'Good use of comments.'

'Thanks. I added those this morning.'

'But you'll delete them after this meeting?'

She nods. 'Buggered if I'm going to maintain them.'

'There is just one thing…'

'Yes?'

'You don't think it's a bit over-engineered?'

'Absolutely. All it needs is a two line properties file. All this extra stuff is way too much.'

'So why do it the hard way?'

'Because we've been told that XML is the company standard for all configuration files. Even when we only need a couple of properties we've got to wrap them in XML.'

'But that still leaves a lot of extra complexity in what you've coded.'

'That's another spin-off from a company policy they're trialling with our team.'

'What's that?'

'Productivity's being measured in lines of code. The more lines we code the more productive we are.'

I ponder on this for a few seconds. 'There is one thing that I think could improve your code,' I tell her finally. 'The XSL file, it's not likely to change very much is it?'

'No, not really.'

'Then why don't you create the file in code, spool it out at run-time and then read it back when you do the transform.'

She nods thoughtfully. 'I hadn't thought of that,' she admits.

'And while you're at it, why use a transform at all? I'm sure you could knock up your own custom XML parser class…'

'Thanks, I'll do that.'

'You see,' I tell her, 'these code reviews really do work. Experience being passed on to a new generation. Best practices here we come. At least until they figure out that lines of code is the crappest measure known to human-kind.'


Who Is Joe Bloggs? Read other episodes here

Return to home page

Contents copyright of Pan Pantziarka. If you like it link it, don't lift it. No copying for commercial use allowed. Site © 1999 - 2007.