Thursday 19 November 2015

Guest author: Why good developers write bad code

A while back one of my mates said there was a topic they had some thoughts on, and figured they should write a blog article about it. Their problem being they didn't have a blog. This seemed like a good way for me to get someone else to do my work for me, so I raised my hand to host it for them here. I've been waiting for... god... oh about three months for this. But 'ere' 'tis:

So there's a guy on your team, and he's a good developer. He knows the language inside out, he's up-to-date with all the latest technologies, he spots design patterns and code smells, and when you send him your code to review he cuts you a new asshole (and it turns out he's bloody right about everything)

... but then he gets a chunky piece of work and, more often than you'd expect, it goes astray. He's swearing at his screen, he's "almost done" for ages. Because it's late the code reviews are lighter than they should be, and QA raise a ton of bugs. He says they're all small but each one takes longer than the next to fix. When you try and spread out the bugfixing among the team it takes everyone else even longer so he ends up slogging through everything on his own. Eventually it's done and it works, but it's brittle and everyone is afraid to touch it. He blames it on over-complicated requirements and over-ambitious timelines and we all move on.

Maybe it was The Business's fault for coming up with stupid requirements, or just the fact that smart people can make mistakes in ingenious ways. Everyone gets unlucky now and again, but if a competent developer gets bogged down regularly then there's something else going on. What is it?

Good devs are often tinkerers, and are very good at figuring out ways to work around obstacles. The dark side of this is that obstacles can be seen as things-to-be-worked-around rather than problems to be solved.

At the code level this manifests as a reluctance to refactor. Tests make refactoring safer and easier, but even with good tests developers often see existing code as fixed, something to be built upon rather than restructured. The upshot of this is you get layers and layers of code, each layer adding a missing piece or working around a bad design decision in the one below, and the complexity quickly gets out of hand. This gets especially bad when time is tight, and guys don't want to go back to the start, and so treat their own almost-but-not-quite working code from a few days ago as immutable (especially after their feature has been through QA) - essentially they get into trial-and-error mode, and nothing gets re-thought, so underlying issues with their code design don't get fixed.

You also see it one level up from the code - when confronted with an over-complicated set of requirements, a capable developer will often just implement them as they are, logical contradictions and all. He'll bitch and moan, but will very seldom chase down a bad requirement and get it (or his interpretation of it) fixed so that it makes sense. We all bang on about elegance, but the requirements are the soil the code grows in, and without taking some responsibility for them your code cannot ever be consistently good.

There's no easy way to fix this. It's hard enough to find programmers capable of writing good code, harder still to find ones that consistently do so. If this is you, write tests you can trust then refactor refactor refactor. If for some reason you're not doing TDD then refactor anyway. If you've coded yourself into a corner create a new branch and work out a new approach in that. Be bold. It's just code, it's not set in stone, and neither are your requirements, so stop your moaning and take responsibility for them. And stay out of trial-and-error mode as much as you can.

If this is someone on a team you lead, TDD and code reviews (especially if they're done early enough to prevent a dev going too far down the wrong path) will help, also because bugs are often fixed by trial and error it's a good idea to keep new features *out* of QA for as long as possible. Really though there's no magic bullet - you'll just have to know who's susceptible to this kind of thing, and keep an eye out for it.

Yep, very true. I encounter this challenge all the time, and indeed sometimes I self-identify as having this problem too. I suppose we all do/did at some point.

Quite often when I receive feedback in my code reviews my initial reaction is "you, I'm right and you're wrong!" Fortunately I've built at least a small amount of self-awareness about this, so I try to keep that comment to myself, and then go "OK, so let's have a look if they're right..?" And do you know what? About 75% of the time they are. This is not an indictment of my code (hmmm... maybe it is?), it's just that a second and/or third set of eyes on one's code is gold, and it should be actively solicited. Code review is as critical a phase of coding as the initial planning and TDD is. It's hubristic for even the best coder to think they get everything right first time, or their code is clear, or they've seen all the refactoring opportunities. Or even that they read the requirements and acceptance criteria properly! Or as my mate says: whether or not they sanity checked the requirements, and they make sense. Bear in mind the person writing the requirements is just as fallible as anyone else, and often the dev team have a better understanding of the system as a whole than people focusing on the minutiae of a single requirement.

Speaking of requirements... it's far too easy for a keen developer to charge off down a rabbit-hole of unnecessary work. I've seen a lot of code that's written like an enterprise Java application (ie: over-written to f***), when we are working with a dynamic language which specifically exists so as to mitigate that sort of coding approach. Just write the code you need to fulfill the current requirement. Don't engage in "what if?", or "wouldn't it be nice if?" or other thought processes that focus on the goal and not the ball. Focus on the ball. Most "what ifs" never eventuate, so one should not bog one's self down with coding for it. It's a waste of time, it adds unnecessary moving parts to what will likely be an already complex situation, and will, over-all probably be counter-productive. The best thing to do is to write short, testable (and tested!) methods, so when it comes time to refactor stuff later on, it's easy. But wait for that requirement to present itself first. Never do any work without some sort of "ticket" (like a Jira issue or similar) - with the ticket sanctioned by someone who's job it is to decide what work needs to be done (who is often not the developer). Make sure the ticket has some sort of planning, and clear acceptance criteria, and then... fulfil those criteria. Don't do anything extra. The complexity might ultimately come, but wait until it's needed. Don't second-guess it being required.

I guess the lesson here is not to be too dogmatic about one's code, or one's coding practices. Always keep one's mind open, and always listen to advice, and give it the once-over and then twice-over before reacting to it. This isn't just "nice team politics", it's critical to the maintainability of one's codebase, which in turn impacts the effectiveness one has at doing one's job. Not a "nice to have". It's essential.