Title: Wallowing in Filth

Author: Martin Moene

Date: 08 May 2015 14:17:03 +01:00 or Fri, 08 May 2015 14:17:03 +01:00

Summary: Pete Goodliffe wades into the dreaded cesspit of ‘low-quality code’.

Body: 

As a dog returns to its vomit, so fools repeat their folly.
Psalms 26:11

We've all encountered it: quicksand code. You wade into it unawares, and pretty soon you get that sinking feeling. The code is dense, not malleable, and resists any effort made to move it. The more effort you put in, the deeper you get sucked in. It’s the man-trap of the digital age.

How does the effective programmer approach code that is, to be polite, not so great? What are our strategies for coping with crap?

Don’t panic, don your sand-proof trousers, and we’ll wade in...

Smell the signs

Some code is great, like fine art, or well-crafted poetry. It has discernible structure, recognisable cadences, well-paced meter, and a coherence and beauty that make it enjoyable to read and a pleasure to work with.

But, sadly, that is not always the case.

Some code is messy and unstructured: a slalom of gotos that hide any semblance of algorithm. Some is hard to read: with poor layout and shabby naming. Some code is cursed with an unnecessarily rigid structure: nasty coupling and poor cohesion. Some code has poor factoring: entwining UI code with low-level logic. Some code is riddled with duplication: making the project larger and more complex than it need be, whilst harbouring the exact same bug many times over. Some code commits ‘OO abuse’: inheriting, for all the wrong reasons, tightly associating parts of code that have no real need to be bound. Some code sits like a pernicious cuckoo in the nest: C# written in the style of JavaScript.

Some code has even more insidious badness: brittle behaviour where a change in one place causes a seemingly unconnected module to fail – the very definition of code chaos theory. Some code suffers from poor threading behaviour: employing inappropriate thread primitives or exercising a total lack of understanding of the safe concurrent use of resources. This problem can be very hard to spot, reproduce, and diagnose, as it manifests so intermittently.

(I know I shouldn’t moan, but sometimes I swear that programmers shouldn’t be allowed to type the word thread without first obtaining a licence to wield such a dangerous weapon.)

Be prepared to encounter bad code. Fill your toolbox with sharp tools to deal with it.

To work effectively with alien code, you need to able to quickly spot these kinds of problems, and understand how to respond.

Wading into the cesspit

The first step is to take a realistic survey of the coding crime scene. You arrive at the shores of new code. What are you wading into?

The code may have been given to you with a pre-attached stigma. No one wants to touch it because they know it’s foul. Some quicksand code you discover yourself when you feel yourself sinking.

It’s all too easy to pick up new code and dismiss it because it’s not written in the style you’d prefer. Is it really dire work? Is it truly quicksand code, or is it merely unfamiliar? Don’t make snap judgments about the code, or the authors who produced it, until you’ve spent some time investigating.

Take care not to make this personal.

Understand that few people set out to write shoddy code. Some filthy code was simply written by a less capable programmer. Or by a capable programmer on a bad day. Once you learn a new technique or pick up a team’s preferred idiom, code that seemed perfectly fine a month ago is an embarrassing mess now and requires refactoring.

You can’t expect any code, even your own, to be perfect.

Silence the feeling of revulsion when you encounter ‘bad’ code. Instead, look for ways to practically improve it.

The survey says...

There are many techniques you might employ to navigate a path into an unfamiliar codebase. Consuming the documentation (or noting the lack thereof), understanding the build/test/deploy process, spelunking the version control information, and code visualisation tools are all useful headstarts. But the only true way to start learning a piece of code is to start physically working with it.

As you do so, you build a mental model of the new piece of code. And then you can truly begin to gauge its quality using benchmarks like:

• Are the external APIs clean and sensible?
• Are the types used well chosen, and well named?
• Is the code layout neat and consistent? (Whilst this is certainly not a guarantee of underlying code quality, I do find that inconsistent, messy code tends also to be poorly structured and hard to work with. Programmers who aim for high-quality, malleable code also tend to care about clean, clear presentation. But don’t base your judgment on presentation alone.)
• Is the structure of cooperating objects simple and clear to see? Or does control flow unpredictably around the codebase?
• Can you easily determine where to find the code that produces a certain effect?

It can be hard to perform this initial survey. Maybe you don’t know the technology involved, or the problem domain. You may not be familiar with coding style.

Consider employing software archaeology in your survey: mine your revision control system logs for hints about the quality. Determine: how old is this code? How old is it in relation to the entire project? How many people have worked on it over time? When was it last changed? Are any recent contributors still working on the project? Can you ask them for information about the code? How many bugs have been found and fixed in this area? Many bugfixes centred here indicates that the code is poor.

Working in the sandpit

You’ve identified quicksand code, and you are now on the alert. You need a sound strategy to work with it.

What is the appropriate plan of attack?

• Should you repair the bad code?
• Should you perform the minimal adjustment necessary to solve your current problem, and then run away?
• Should you cut out the necrotic code and replace it with new, better work?

Gaze into your crystal ball. Often the right answer will be informed by your future plans. How long will you be working with this section of code? Knowing that you will be pitching camp and working here for a while influences the amount of investment you’ll put in. Don’t attempt a sweeping rewrite if you haven’t the time.

Also, consider how frequently this code has been modified up to now. Financial advisors will tell you that past performance is not an indicator of future results. But often it is. Invest your time wisely. This code might be unpleasant, but if it has been working adequately for years without tinkering, it is probably inappropriate to ‘tidy it up’ now, especially if you’re unlikely to need to make many more changes in the future.

Pick your battles. Consider carefully whether you should invest time and effort in ‘tidying up’ bad code. It may be pragmatic to leave it alone right now.

If you determine that it is not appropriate to embark on a massive code rework right now, that doesn’t mean you are necessarily left to drift in a sea of sewage. You can wrestle back some control of the code by cleaning progressively.

Cleaning up messes

Whether you’re digging in for the long haul, or just making a simple fix-and-run, heed Robert Martin’s advice and follow ‘the Boy Scout Rule’: Always leave the campground cleaner than you found it. It might not be appropriate to make a sweeping improvement today, but that doesn’t mean you can’t make the world a slightly less awful place.

Follow the Boy Scout Rule. Whenever you touch some code leave it better than you found it.

This can be a simple change: address inconstant layout, correct a misleading variable name, simplify a complex conditional clause, or split a long method into smaller, well-named sub-functions.

If you regularly visit a section of code, and each time leave it slightly better than it was, then before long you’ll wind up with something that might be classified as good.

Making adjustments

The single most important advice when working with messy code is this:

Make code changes slowly, and carefully. Make one change at a time.

This is so important that I’d like you to stop, go back, and read it again.

There are many practical ways to follow this advice. Specifically:

• Do not change code layout whilst adjusting functionality. Tidy up the layout, if you must. Then commit your code. Only then make functional changes. (However, it’s preferable to preserve the existing layout unless it’s so bad that it gets in the way.)
• Do everything you can to ensure that your ‘tidying’ preserves existing behaviour. Use trusted automated tools, or (if they are not available) review and inspect your changes carefully; get extra sets of eyeballs on it. This is the prime directive of refactoring: the well-known set of techniques for improving code structure.

  This goal can only be reached effectively if the code is wrapped in a sound set of unit tests. It is likely that messy code will not have any tests in place, so consider whether you should first write some tests to capture important code behaviour.

• Adjust the APIs that wrap the code without directly modifying the internal logic. Correct naming, parameter types, and ordering; generally introduce consistency. Perhaps introduce a new outer interface – the interface you wish that code had. Implement it in terms of the existing API. Then at a later date you can rework the code behind that interface.

Have courage in your ability to change the code. You have a safety net: source control. If you make a mistake, you can always go back in time and try again. It’s probably not wasted effort, as you will have learnt about the code and its adaptability in doing so.

Sometimes it is worth boldly ripping out code in order to replace it. Badly maintained code that has seen no tidying or refactoring can be too painful and hard to correct piecemeal. There is an inherent danger in replacing code wholesale, though: the unreadable mess of special cases might be like that for a reason. Each bodge and code hack encodes an important piece of functionality that has been uncovered through bitter experience. Ignore these subtle behaviours at your peril.

An excellent book that deals with making appropriate changes in quicksand code is Michael Feathers’ Working Effectively with Legacy Code. [1] In it, he describes sound techniques to introduce seams into the code – places where you can introduce test points and most safely introduce sanity.

Bad code? Bad programmers?

Yes, it’s frustrating to be slowed down by bad code. The effective programmer does not only deal well with the bad code, but also with the people that wrote it. It is not helpful to apportion blame for code problems. People don’t tend to purposefully write drivel.

There is no need to apportion blame for ‘bad’ code.

Perhaps the original author didn’t understand the utility of code refactoring, or see a way to express the logic neatly. It’s just as likely there are other similar things you do not yet understand. Perhaps they felt under pressure to work fast and had to cut corners (believing the lie that it helps you get there faster; it rarely does).

But of course, you know better.

If you can, enjoy the chance to tidy. It can be very rewarding to bring structure and sanity to a mess. Rather than see it as a tedious exercise, look at it as a chance to introduce higher quality.

Treat it as a lesson. Learn. How will you avoid repeating these same coding mistakes yourself?

Check your attitude as you make improvements. You might think that you know better than the original author. But do you always know better?

The Curious Case of the Container Code

There was a container class. It was central to our project. Internally, it was foul. The API stank, too. The original coder had worked hard to wreak code mischief. The bugs in it were hidden by the already confusing behaviour. Indeed, the confusing behaviour was a bug itself. One of our programmers, a highly skilled developer, tried to refactor and repair this container. He kept the external interface intact, and improved many internal qualities: the correctness of the methods, the buggy object lifetime behaviour, performance, and code elegance. He took out nasty, ugly, simplistic, stupid code and replaced it with the polar opposite. But in his effort to maintain the old API, this new version was internally far too contrived, more like a science project than useful code. It was hard to work with. Although it succinctly expressed the old (bizarre) behaviour, there was no room for extension. We struggled to work with this new version, too. It had been a wasted effort. Later on, another developer simplified the way we used the container: removing the weirder requirements, therefore simplifying the API. This was a relatively simple adjustment to the project. Inside the container, we removed swaths of code. The class was simpler, smaller, and easier to verify. Sometimes you have to think laterally to see the right improvement.

I’ve seen this story play out many times: a junior programmer ‘fixed’ a more experienced programmer’s work, with the commit message ‘refactored the code to look neater’. The code indeed looked neater. But he had removed important functionality. The original author later reverted the change with the commit message: ‘refactored code back to working’.

Conclusion

You can do nothing but expect to be presented with ‘bad’ code at some point. You have to learn good, mature, ways to deal with it. That is the lot of the professional programmer.

Ultimately, we must heed the valuable advice mentioned earlier: employ the Boy Scout Rule. Leave every piece of code you touch better, if even only fractionally.

Questions

• Why does code frequently get so messy?
• How can we prevent this from happening in the first place? Can we?
• What are the advantages of making layout changes separately from code changes?
• How many times have you been confronted with distasteful code? How often was this code really dire, rather than ‘not to your taste’?

References

[1] Michael Feathers, Working Effectively with Legacy Code, Upper Saddle River, NJ: Prentice Hall, 2004

Notes: 

More fields may be available via dynamicdata ..