My thoughts on “bad code”
twitter.com
twitter.com
There's a real danger in a well-intentioned clean code advocate. First, because there are no universal code principles. Maybe "have it work", but even then, a lot of value has been gotten out of code that doesn't work. And second, because it's very easy to go from a good guideline to a dogma. All you need is a few people to misinterpret your guidelines. Heck, a lot of people haven't even read the guidelines. They just parrot what someone else paraphrased on Twitter.
Fast forward about 11 years, and people realize that having a million nested if statements and matching else blocks is actually absurdly unreadable; much better to just return early, avoid nesting, and break the "only one exit" pattern.
function doSomeStuff(input) {
if(someInvariantIsUnsatisfied(input)) {
return Error;
}
if(someOtherInvariantIsUnsatisfied(input)) {
return Error;
}
//do stuff
return result;
}EDIT: Exit the body of the guard statement, not enter.
I get you’re going for the non-standard markdown strike through, but that looks confusing and calls attention to something you meant to remove. You can (well, could) remove the word when editing.
Especially when the code to do that depends on the amount of locals, having only a single place where you assign space for locals and a single one where you revert that helps a lot.
It also can help in languages that don’t help you run cleanup code (closing files, freeing memory) at function exit. Change the code and forget to update one of your early exits, and you have a bug. If that exit is rare, that may ship and may become a vulnerability.
https://github.com/torvalds/linux/blob/master/Documentation/...
int foo(int bar) {
int retVal = -1;
if (bar == 1) {
retVal = 1;
}
if ((retVal % 2) == 0) {
retVal = 2;
}
return retVal;
}Also, putting code inside a do ... while( false ) loop purely so you can use break as a sneaky goto to avoid deeply nested conditionals while technically adhering to the single return rule.
I think when you're starting to use control statements in bizarre ways like that, it's a good indication that maybe that's a case where breaking the "rules" is the best thing.
Single Return made sense when we had to de-allocate resources, but there are few years already that most languages have memory safe resource-counted allocations (C++11 for i.e) that will free resources correctly independent of your single/multiple returns.
I don't believe that keeping those inherited "best practices" from the past will help us developing modern code.
Long life to Clean Coding!
Yes. This rule was an overreaction to a pervasive problem back in the day.
But, much like the use of "goto"s, there are times where your code is more readable, maintainable, and efficient if you ignore the prohibition. The trick is to know when that's the case.
What we have to keep in mind is that the needs of whatever codebase we're working on might be out of sync with fashion. For example, I've been relishing the recent push-back against DRY, because I think it's over-applied in damaging ways, but the main codebase I've been working on for the last year needs more application of DRY, not less.
I think it's fair to say that AbstractSingletonProxyFactoryBean[0] is something of an abomination, but it's important to avoid throwing the baby out with the bathwater. Some of the Go4 patterns like Command, Observer, Visitor, and Iterator are literally all over the place and you have to actively go out of your way to avoid writing them.
Likewise, Haskell type classes aren't something you're supposed to be implementing yourself all over the place in your application. They're very high bang-for-buck things you implement in libraries. The equivalent to implementing iterators for your collections, or AutoCloseable on resource-like java classes.
0. https://docs.spring.io/spring-framework/docs/current/javadoc...
I’d say it’s pretty subjective. I’m the opposite. I can’t read functions that are a hundred lines long with a dozen bound variables and half a dozen branches and early exits. That’s not easy to read and it requires keeping a notepad with you to figure out the control flow.
I much prefer short definitions with clear logic. Leave the semantics to the edges of the program where it matters. It’s easier to reason about using substitution and algebraic manipulation.
Don’t get me wrong though, I write my fair share of C code and low level drivers. For that though I tend to use a higher level language to work out the logic.
My business isn’t writing code. That’s the boring part. It’s solving the problems that is interesting.
But that’s the funny thing. Regardless we could both solve the same problems in our own ways and more often than not it will be good enough.
After twenty years and hoping for another twenty… I guess I’ve learned that what matters is that you can get along with your team and read each other’s code. Styles come and go and don’t matter that much.
It does require a notepad as well when a five-page long function gets smeared across three “services”, five files and tens of functions, each doing a couple lines of code.
Leave the semantics to the edges of the program where it matters
Agreed. But when I meet this highly-structured code (and get confused), it’s usually split not across semantics, but across some random undocumented ideas about reuse, undeclared abstractions, highly indirect flow control, etc. And all of that named as if it was a most bizarre naming contest.
So that instead of a little hairy idea you get a convoluted irrational multi-plane horror to deal with.
It requires discipline to achieve what I call here, principled combination: that the combination of functions follow certain laws like they do in mathematics. In many programming languages it requires discipline because the type systems and the programming languages are not constructed in such a way as to uphold these laws for you. You, the programmer, have to enforce these laws on your code.
That is why, when I'm writing something in C, I will some times prototype my design in a higher-level language first and translate that into C code. The higher level language is more principled and helps me catch errors in the design and development stage before I've made a mess of things.
I don't always do it that way but it tends to work the best when I can get it to.
People praise typed languages, but honestly I’m much more okay with dynamic typing than with the lack of beams and pillars for design, to do any of it. It’s very easy to lose yourself for an hour and start creating a mess in what you have planned for weeks, simply because your attention was elsewhere, there was a deadline or new goals were incompatible with current ideas and you had no clue how to marry them correctly (nor courage to begin) within a timeframe available.
Add a couple of developers who aren’t you and/or switch into another project for a month or two – and it’s a recipe for a mudball. I used to believe in all that, not anymore. May be a bold claim, but most projects aren’t even hard enough to require it. I remember my own past experience and can tell that all that division into layers, concerns, etc was not a requirement nor an enhancement. I was simply ticking boxes in a “how clever do you feel this week” form. Days of work to spare five pages of clear actual instructions which could have been typed and tested in under few hours, shipped next evening and – much more importantly – I could reenter that context in few minutes after a year off, in contrast to any to-be-mudball.
I like to imagine each execution path through a function as a piece of string. The fewer strings, the fewer kinks, and the less string overall, the easier it is for my monkey brain to handle most of the time. Yes this is 'cyclomatic complexity', but strings are nicer visually :)
Another issue that's never explicitely mentionned is the vastly different calibers of programmers and engineers in the field.
I'm pretty sure John Carmack, Linus Torvald or Donald Knuth's definition of "good code" is quite different than $BODY_SHOP_RESSOURCE_1389's.
I actually would not. I do not want to have to read every detail of each condition or formula on the first pass. I want to see high level gist of what is supposed to happen - high level algorithm. Plus, I like when variables have clearly limited scopes.
If I have a detailed, firm spec that I know won't change at all, I'd spend some time planning and then build a DRYed up, abstracted, well-tested, etc solution. I know the solution is going to be sticking around for a while, more or less, so it makes sense to devote the effort to build a solid foundation.
If the thing I'm building is squishy, looking for heavy user feedback, will likely be iterated on quickly (i.e., very agile manifesto agile), I won't spend the overhead time for those things. There's a nonzero chance the code ends up in the trashcan, and the most important thing is to ship something and get feedback from real customers. You later go back and clean it up, based on the growing confidence in the permanence of that feature.
I am okay with copy-pasting code but after the second or third copy-paste, you should probably at least consider asking, “Should I abstract this into its own function?”
One copy of something is YAGNI. You aren't going to need it (an abstraction).
Two copies of something are coincidence. It's fine to copy and paste and leave it that way.
Three copies of something are finally a pattern. At least three copies are when you start to really see what sort of abstraction that you need to handle the pattern.
Well, yeah, coz that's worse.
It decreases code cohesion (especially when those functions are stuck in files far away) and to little appreciable benefit.
>First, because there are no universal code principles
I think there are and good developers have a spidey sense about what they are and will usually agree but we've yet to culturally agree on what they are as an industry.
Moreover, some literature on the topic (e.g. Robert Martin) is very, very wrong.
They rather have clean, SOLID, DRY, design patterns than easy, fast, productive code.
Familiar thing often “feel better” to us even when they’re quite bad. I’ve seen that bias at play when new people join a team.
Their code uses too many classes? Bad code! Its overly factorized? Under factorized? Too many dependencies? Too much reinvention of the wheel? All these things are obviously evidence that whoever wrote this code is inexperienced (and they probably have deep character flaws).
I'm sure other people feel exactly the same way about my code, too. Its frustrating and dumb.
Me too. But...
Every so often it happens that I read bad code, with functions that are too long and complex or too short, or with misleading variable names, or a way too clever class structure, or whatever; cursing the person who wrote that code ... only to find out it was me who wrote that code.
It makes me put things in a different perspective. I try to judge not too harshly.
But if it’s any consolation, it’s pretty much the same with any cognitive bias: you can never totally eliminate them, but you can make progress, and the first step is just knowing about it! :)
"Bob doesn't like it when I do this but fuck him"
That’s the only situation where you get enough direct, useful feedback over time on whether something is good or not.
This is actually better because you’re bringing the person back to the forefront instead of trying to judge code as objectively good or not
This could mean working with (competent) people who think differently, or working with a different language (my most formative was Clojure, as someone who had never even seen a lisp before), etc.
Initially I bend over backwards in near-total deference, and over the course of a few months I start to feel like I’ve gained perspective. It’s the only way I’ve found to not carry the baggage of “my way” into new situations.
For example recently learning RxJS and writing a complete mess of state spread across my app feels good because its "declarative" compared to the ugly previous "imperative code".
I think part of it is the reward of the learning process itself, but I can see it being an addictive cycle.
It's like Stockholm syndrome for developers.
After all how could code be bad if it follows a pattern?
public class C {
int x;
public C(int x) {
int x;
this.x = x;
}
}
"Sometimes I shake my head and wonder. And sometimes, I just shake my head."
It's one thing for a junior dev to do this. But try working with a bunch of amateurs that somehow got promoted to lead because they kind of worked with some technology at their prior job. It's amazing watching a company replace domain expertise with superficial framework-of-the-day knowledge. Especially knowing that these devs will move on in a year escaping the consequences of their actions.
I think it would be unwise to suggest bad code is good without qualifying that 'good' here just means 'better than terrible', which is my more literal interpretation of the tweet.
I'm also not fond of the idea that bad code is not necessarily bad because of xyz circumstance that lead to the bad code. Can't we just call a spade a spade? We all have or will continue to write code which is less than ideal; seems like too much ego is attached to the code and we're gonna start down the All Code Is Beautiful path. Sometimes you just gotta write garbage, but garbage is garbage at the end of the day.
I’ve been in extremely frustrating situations where horrible horrible code gets steamrolled into production systems because of <arguments>. However, in nearly every single case that wasn’t a well-intentioned junior, the bad code was almost never the result of following bad advice: it was the result of the engineer not caring.
Start with caring and empathy, and always worry about how “nice” your code is. That’s how it’ll get better.
His argument, summarized, is that ostensibly "bad" code is not as bad as it could be, because that implies you can readily tell what it does, immediately see issues with it, and know how to improve it. Possibly, this code might not fully deserve the label "bad". It's _far_ better than code that's completely impenetrable.
Reminds me of the Bjarne Stroustrup quote: "There are only two kinds of languages: the ones people complain about and the ones nobody uses".
* You can't express `T` where `null` is forbidden in the type system so you get NullPointerException everywhere and defensive null checks.
* You express a sum type as a product type because your language does not have sum types .
* Your language doesn't have first class multiple return values (or tuples) so you return extra parameters via out parameters or thread local variables such as `errno`.
* Your language doesn't have exceptions (or algebraic effects) and can't do IO so you have monad transformers.
* Your language doesn't have set-theoretic types so you need hacks like `thiserror` .
* Your language doesn't have stackful coroutines or can't infer async IO for you so you have `async/await` spam or callback hell or "mono's".
* Your language doesn't have exhaustive checks (or pattern matching) so you need a fallthrough case check on switch statements .
* Your language doesn't have algebraic effects, so you need to pass context everywhere.
I know someone will reply about Java's null annotation checking options, so here is one of them: https://github.com/uber/NullAway .
I would love to see a future where folks write code without thinking about how "scalable" or "performant" their code is and instead focus on how easy it would be to change the behaviour of existing code.
At one point experience and intelligence converge and things get better.
You may not be running your site on a pentium 2 but shitty performance adds up quickly and computing power is not infinite.
You have to be skilled at office politics or make sacrifices to keep code quality high. Usually not worth it. You get yelled at less if you externalize all the costs to future suckers, instead of trying to reduce those costs upfront.
I literally just had this exact meeting yesterday.
So much basic maintenance had been skipped that a suite of apps was costing too much money to migrate to a new platform. Instead of asking for more budget for that app, the dev manager suggested using the money from an upcoming project for an unrelated app to finish off this one!
It's a literal Ponzi scheme, using funds from newcomers to pay off the technical debts of previous projects.
Which of course never ends. Now the next project will go over budget, leading to rushed work, corner cutting, and drawing down on the project after that, which in turn will be a mess, etc...
It's fascinating to watch this kind of stuff unfold as an outsider.
Just try really hard to not work for shortsighted people or bad companies. There are tons of great workplaces out there that will treat you like a king and pay you very well. Keep looking, these “almost too good to be true” companies exist. Vote with your feet at every opportunity.
This has been my principal design consideration for a while now. Not "the code should be short!" or "the code should be elegant!" but "it should be easy to change" because your requirements are always going to change. It just so happens that the latter usually means things like single responsibility and loose coupling that bring about the former as a matter of course.
Of course scalable and performant is nice. I have also been on teams that delivered absolutely nothing because the KPIs were more important than the product.
What properties of the code need improvement or demonstrate what other should aspire to? -- Getting someone to really articulate that is quite difficult.
Readability definitely has objective measures. One that I find especially interesting goes by the term "cognitive complexity" and measures, in part, comprehensibility. A 2003 article in the Canadian Journal of Electrical and Computer Engineering titled, "A new measure of software complexity based on cognitive weights", defines it as "the degree of difficulty or relative time and effort required for comprehending a given piece of software modelled by a number of BCS [basic control structures]" In 2018, "Cognitive complexity: an overview and evaluation" in Proceedings of the 2018 International Conference on Technical Debt gave a 3-part criteria for evaluating complexity.
Not that this is not the same as cyclomatic complexity, a measure best suited to gauge the effort needed to adequately test a system.
If you assume nobody is looking, you will often write bad code. If you assume that someone is looking, possibly including Future You, you will aim to write good code.
- “It passes the linter so don’t give me any advice on how it can be improved”
- “YAGNI”
While both of the above could be good counter-arguments in some contexts, I’ve seen them used disproportionately often by set-in-their-ways senior engineers who don’t care anymore or who forget that we write code for other humans, not for machines.
Many people ragging on the founder|coder for writting a clean snippet of linear search function when he obviously should have done better and used a binary search (or some other algo) .. ergo "Bad Code!!".
What was missing from that was any context about the volume of expected data per "need to seach this" call etc.
It's entirely possible that every instance was to quickly search an already primed cache of data and that the optimal performance on the target hardware was to minimise branching and linearly rip through a small chunk of data in a load page.
Or, maybe not.
Point being, context and bigger picture plays a part here also.
https://stackoverflow.com/questions/65753731/why-searching-a...
Hardly clean, I think you'd agree.
In any case, you shouldn't use explicit iteration to solve this problem without a good reason. Every language should have both linear and binary search as part of its standard library.
I agree that you shouldn't especially choose one algorithm over another without reason and I feel comfortable choosing a linear scan of short sections of cached data in pipelined situations where branching overheads are steep.
Was the case in the TV episode such a scenario? I have no knowledge of that myself.
Vouched.
Also, if you're on a tight timeline for a critical function, you may want to push ugly/inelegant/dense/"bad" code just to get the function in prod a day sooner. This doesn't mean the code isn't "bad," it just means that the business needs outweigh the ugliness.
Then I was told the CEO and CFO wanted a preview in 3 days....
That was 12 years ago, still in production as far as I know!
[The functionality was supposed to be replaced with something built in or integrated with the new ERP system.... but that project crashed and burned].
It's ugly, it's a maintenance headache, but it does function, and very robustly too. There are no API calls, no microservices, no performance issues, etc...
1. “I see something that makes it hard to work on this code”
2. “I don’t understand what the code is doing and touching anything breaks everything.”
Code can be bad on many levels. It can be bad on the level of some one liner where someone appends to a list in javascript by using the length as an index. It can also be bad on the level of being an impenetrable wall of shit that is impossible to understand.
And just because I don't understand how something works doesn't mean I don't know why it's bad. I know why it's bad because I know it can be done simply but the person that implemented decided to write their own terrible and over engineered design.