Goodbye, Clean Code (2020)
overreacted.io
overreacted.io
While undeniably true, this feels completely orthogonal to the question of clean code. You could ruffle someone's feathers in the exact same way by taking code into the opposite direction.
"Goodbye, Clean Code [...] Don’t be a clean code zealot"
I agree with choosing pragmatism over purism. But the clickbaity title exercises a pattern I've seen hundreds of times, to which I have frankly become a bit allergic over the years: "X considered harmful! ...I mean, if you're overdoing X". Not being a healthy food zealot doesn't equal "goodbye, healthy food".
(Putting common sense in scare quotes to acknowledge that this is new information to some, no matter how obvious to others)
That said, despite the clickbait title, I think these sorts of posts make a valid point because people often do in fact take heuristics like DRY to mean "never repeat any code under any circumstances and any code repetition is per se bad."
So it may be good to remind ourselves "Hey, this is just a thing. Use it when it makes sense, and don't overdo it."
I think this mentality and the Clean Code Movement (with capital Cs) actually have a lot in common.
Much of the Clean Code movement is occupied with the idea that there is one, single, right way of doing things. Anyone who does things differently is doing things the wrong way.
There is a subset of programmers who are attracted to these movements because it can feel like a free license to feel superior to your peers. Once you understand all of the rules and intricacies of the Clean Code Movement, it can feel like a free pass to push the Clean Code rules on to others and the codebase. No need for code review or consultation, because you've already decided that the Clean Code Way is the right way and therefore there isn't anything to discuss.
Well if this is the case then they certainly didn't get it from the Clean Code book itself. The books very explicitly makes the point that there's no one right way and that the guidelines in the book won't apply to every situation.
> Consider this book a description of the Object Mentor School of Clean Code. The techniques and teachings within are the way that we practice our art. We are willing to claim that if you follow these teachings, you will enjoy the benefits that we have enjoyed, and you will learn to write code that is clean and professional. But don't make the mistake of thinking that we are somehow "right" in any absolute sense. There are other schools and other masters that have just as much claim to professionalism as we. It would behoove you to learn from them as well. [emphasis in original]
I liken the CC movement to religion - it has good intentions, but when taken too far you end up with a holier-than-thou attitude.
For me, one of the interesting points of the article is not just that the replacement code was inferior, it's the recognition that the emotional compulsion to "clean up the dirty code" was a problem in itself.
Sure, as you say, if you refactored it in the other direction in this would also be problematic -- but there aren't many people who feel a visceral emotional compulsion to refactor code to make it less "clean". (Maybe brainfuck aficionados? Anyway, a rare breed.)
So I think there's something about the concept of "neat/clean code" that asymmetrically/directionally produces this error, and I took the "goodbye" to be giving up on this compulsive attachment to the concept of "all code must be cleaned", rather than saying that the concept of clean code shouldn't be used anywhere. The money quote being:
> Am I saying that you should write “dirty” code? No. I suggest to think deeply about what you mean when you say “clean” or “dirty”. Do you get a feeling of revolt? Righteousness? Beauty? Elegance? How sure are you that you can name the concrete engineering outcomes corresponding to those qualities? How exactly do they affect the way the code is written and modified?
It's really more about identifying the feelings that "dirty" code produces and realizing that they might lead you astray.
> "X considered harmful! ...I mean, if you're overdoing X"
I'm 100% on board with the general objection to this kind of thing, it's a pet peeve that I share. I just didn't get triggered by this one :)
That's certainly a valid observation and a good point. I think the article would benefit if it the author analyzed this aspect more.
I really don't see your point about the article being linkbait-y at all.
WHY do we write clean code?
Because the value of software is its ability to CHANGE. Otherwise we would stick with fixed circuits. Best practices and clean code don't exist in a vacuum and they are not floating abstractions that developers should throw around as if everyone is on the same page with respects to what they mean.
They are a set of quality standards that allow for the rapid development, adaptability and ease of maintenance as a codebase evolves over time.
Clean code does not exist to slow or hinder development, or to handicap junior developers. It exists to maintain a team's velocity as the scope and scale of the project increases. Clean code is one of the crucial tools in the toolbox for avoiding death march projects. It it is not the only tool: an efficient management process, feedback loop, QA cycle, deployment process etc. are all critical as well. It is a team effort and code quality is the part that rests directly on the shoulders of the developers.
The one and only part of the blog/article that I agree with is the part about refactoring without making it a teaching moment for the dev who wrote it. That part implies they weren't doing code reviews, another crucial element in the quality control of the product being developed.
It blows my mind that these conversations are so often misunderstood. Us old timey senior devs have a professional obligation to make sure that the younger generation understands these concepts. Why do we see so many trendy medium articles and blog posts that seem to want to throw decades of hard learned lessons out the window? I blame the older generation for failing the younger ones. This stuff is not self-evident and has to be taught.
The problem that highlights is both a process and an experience problem. It indicates that he did not have a clear picture of the development road-map for the project, which suggests there are silos of information. There were a lot of hints about this actually. The lack of code reviews (that's where the original dev's opportunity for quality improvement should have been caught and addressed), the fact that a manager would chastise him for trying to improve code quality and the fact that he made poor design choices when trying to improve said quality really does point to a dysfunctional process.
Pointing the finger at code quality here is failing to understand the difference between the trees and the forest.
I've had joyful experience of working with duplicate code that spanned over multiple thousands of lines. I still fondly remember a PHP script generating multiple xml files, supporting different versions of format. (It was an e-commerce format for exporting and exchanging products data). And with multiple inconsistencies among this copy-pasta that have accumulated over time.
The problem here is the boiling frog syndrome. Duplication rarely starts with thousands of lines. It grows over time, and the longer you put up with it, the harder it becomes to roll it back. But as long as it grows slowly, the frog feels comfortable.
Clean code approach may get overzealous, the potential for fanaticism may even be sort of inherent to this approach (I agree), but it stems for acknowledging this simple, sad truth. The truth is that complexity of naive code is kind of like cancer. If you don't detect it before it actually begins to hurt, it's not great news.
"A codebase where everything is easy to change becomes complex in aggregate"
What you're describing there is a code smell that is commonly referred to as "Speculative generality" and was written about in a book by Martin Fowler called "Refactoring, improving the design of existing code." Which, incidentally is a great companion book to Robert Martin's Clean Code.
Speaking of trendy medium articles, here is one that I wrote on the concept of clean code that might help better understand what I mean when I use the term:
https://medium.com/@gspencley/what-does-clean-code-mean-anyw...
Thousands of small methods spread through hundreds of FactoryFactoryFactories and Builders and Adapters and Visitors can become a maintenance nightmare.
It's almost like you didn't even ready my comment, or completely glossed over the "why do we write clean code" part. Clean code is code that is easy to understand and maintain. If you're describing the opposite then you're not describing clean code.
"It was already late at night (I got carried away). I checked in my refactoring to master and went to bed, proud of how I untangled my colleague’s messy code."
No PR, no code review, no CI. Just a cowboy pushing to the master..
Let's be clear that the dangers here are "no code review" and "no CI".
The others are fine in most circumstances, unless you work on some huge monolithic codebase.
Smaller cross-functional teams should be able to push to master to solve problems then and there without everyone's unanimous approval, and everyone should feel enough safety to be able to make changes to the codebase they share with their team.
The real "root of all evil" is why is someone working against the team from within the team?
We all push to main, no code reviews, no CI etc.
Hell, some people here patch things by downloading the live dll, decompile, update code, recompile and stick it back to live...
https://wiki.c2.com/?ContinuousIntegration " The most granular unit of integration should be one step of one refactor. "
Now, you could bypass all those tests and reviews and just deploy it anyways. And then you'll have a shitty codebase, but you'll have CI so that's good, right?
How is pushing to master not a form of CI?
Assuming Master is the "protected branch" and not kitchen sink, this sounds like - "we test only in production". Wouldn't or shouldn't reviews happen before merging to master?
I worked this way for years before we moved into full branched code review. I would never go back however.
That said, you should still have at least some review and automated testing before your code gets in everyone else's way.
"duplication is far cheaper than the wrong abstraction."
I think this is a journey a lot of software engineers go through. I can say I was walking down this path in the past.
If you treat the new way as an experiment, and only gradually convert the rest of the code, you're in for a smoother ride.
It's totally fine to try out an experiment in a branch and deploy to QA but don't deploy it to prod until you're sure.
This comes from a lack of discipline in the org, not from the parent's (IMO) correct guidance that an org should undertake architectural changes with pilots in isolated parts of the code base.
"hey we need feature X" (feature X is 2 lines of code).
"Ok, I've made 4 layers of abstraction, and 2 interfaces! an extra abstract class!"
Most engineers seem to have the wrong takeaway from this post. Every abstraction goes from right to wrong eventually. The answer is not to not abstract, the answer is to ruthlessly tear down abstractions when they go from right to wrong, which people seem to have trouble doing.
I think the other major problem with abstractions is that they have a cost and no one seems to like to discuss that. Even the right abstraction has a cost.
1. Write _anything_ that works. Dependencies, code quality don't matter. Code is idiosyncratic but can be understood by a reviewer.
2. Apply abstractions _everywhere_. Re-implement data structures and algorithms (maybe unknowingly). Code is now very hard to understand.
3. Figure out one is completely unable to update or even maintain code written 6 months ago. Rewritten code suddenly becomes clearer, one begins to think about programming as an craft and not just hacking things on a keyboard until you get the desired result. Dependencies, judicious comments, code quality and a sane terseness become important. The journey starts here.
Personally, step 1 was very short as I learned to program on the job. I was responsible for code, and so I needed to get it together quick. A language like Python makes 2 very easy, and 3 came very quickly too since, as I mentioned, I was responsible for the code (one man team).
I have met people who work at large institutions who are stuck at 1. Others with degrees in CS who are stuck at 2. Some just "get it" and go straight to 3. But typically, the real journey starts when you need to go on with work but your prior self is preventing you from being efficient. You need to get rid of that prior self's work to move on.
- Kent Beck
It basically comes down to:
1. Write the code that makes the computer do what it needs to do to achieve a specific goal. No more and no less.
2. Refactor ("compress") the code by deriving re-used (implicit) structures and functions.
He puts a lot of emphasis on those steps being done in order and on deferring the second step
Note that this is from someone who writes games/engines/tooling so there is a large, real pressure to write simple and reasonably performant, optimizable code. He would also prefer that most of programming would be done in this way.
I had a first round interview that went well, and was given a take home and the problem was very simple, dead simple. In turn I tried to keep the code as simple as possible, with only necessary abstractions.
The second interview was with an entirely different person who seemed displeased I didn’t bloat the solution with all sorts of enterprise patterns, dependencies and conventions. I was kinda backed into a wall and while attempting to explain my reasons I ended up in an argument with the interviewer and it was clear we did not see eye to eye. He would later challenge my ability to perform in a patronizing tone and I was sure the opportunity was dead
It wasn’t the only thing that went wrong with that interview, but I feel that disagreement was the biggest killer.
I know people who actually believe a typedef for a pointer or atomic type is a Good Idea.
I always talked the talk and walked the walk for the interviews. I know people love Solid, design patterns, Dry, OOP, Restful, Clean Code, complicated architectures. So I am always willing to talk about these and even sprinkle the discussion with the most obscure design patterns they may never heard of.
Yes, I do consider some of these being fads or even detrimental to good software but my goal was to be employed, not to convince people that there is not only one True Way.
Yes, Uncle Bob :)
The only time duplicated code is bad is when there is an actual logical requirement for the multiple instances of duplicated code to be the same. Like some actually underlying concept linking the duplicated code sections that is worth abstracting.
That is not always the case. Just as often IME the situation is actually, "these two things happen to have the exact same behaviour right now." It's important to recognize that there may be no guarantee that will be the case *in the future*.
If the reason for the duplicated code is just coincidence, let the code be duplicated. You'll give the duplicated code sections freedom to drift apart naturally as requirements change and save yourself the trouble of having to decouple things later.
Totally agree! However, if code is duplicated because there is some is some assumption of functionality that is represented in multiple places then it gets more iffy. I took this article to imply that in each function implementation there were something like similar calls to a library or particular orders of redrawing based on the way the code base worked. That's where it can get dangerous.
It's not as dangerous if there are four very similar functions sitting right next to each other. But if there is a block of code that implements a way of working and another block of code has the same implementation in a place that isn't as easy to find that seems dangerous to me. If you want to make one change to a code base ideally you make the change in one place.
I think the true decision function lies somewhere in the middle. For relatively simple code that doesn't need to be in sync then yes, duplication is likely the best option. For complex code or code that should stay in sync then extracting the common code is likely best. But there is a huge range of code in the middle where it is a judgement call with no obviously correct answer.
When it's your own personal project and you can be your own little tyrant, do whatever you want. Be as "clean" as you need to be. But this was at work, and Dan was being a bad coworker.
He snuck in a change in the middle of the night over a coworker's code. This code wasn't his responsibility. He didn't leave his thoughts on a PR where others could discuss it. He overwrote someone else's code without asking. There was no opportunity for discussion or collaboration. Dan's actions said he's right and his coworker is wrong. They said he doesn't trust his coworkers. They said he knows best. They said if you want something done right around here you have to do it yourself. Dan was not being a team player. This story is less about code and more about team dynamics.
I hope Dan knows this and he was just trying to sneak the message past people who don't take social cues as easily as others. I hope his boss didn't just tell him to revert his change. I hope his boss told him why.
EDIT: I just reread the article and I think Dan missed the point. This wasn't about the code, as I said. But Dan really does seem to think it's about the code.
In the best case scenario they all slighty change over time without forcing complexity on each other. But it becomes a problem as changes that should be breaking only break part of the code, and the rest can go unfixed as nobody remembers all the linked bits.
It would be critical for instance if the duplicated bits were involved in invoicing procedures, and one in five remained unchanged while the other got updated.
// @note this code is duped in ../../some/other/file.code
This difference is something important to stress.
Additional logic is only needed when code is almost duplicated, and that is where the question arises of whether what is happening is best viewed as a different “version” of some common process, or a different basic process that has similarities.
Actual duplication doesn't require additional logic to remove duplication; and even near dupes often don’t require distinct (i.e., branching) logic if the language offers the right abstraction facilities.
Refactoring is fine. Abstraction is fine. But programmers fall to, at some point in time, some sort of tunnel vision that drives us to use either tool to target the wrong thing.
At my current job, we have this tool to create query abstractions, a Spring Data of sorts if you will. It seems nice, and it naturally feels better than the alternative, which is allow the upper layers to issue queries themselves.
But it is wrong. All flexibility is gone. Our team is just not big enough to expand the functionalities of such library, so the queries it can handle are pretty basic. Also, there are at least five layers to go through in order to debug it, and the only thing it really does is converting objects into queries.
Of course, most API consumers have to bypass the library. It is just not flexible or powerful enough.
Moral of the story is, to accomplish a big refactor, having a clear, global vision of the long term benefits and shortcomings, is essential. Refactoring just for the sake of making something look good, is not worth the time.
"Abstract over data, not behaviour"
This seems to fall into the trap of abstracting over behaviour.
The best other example I've thought of for this is the 'generic repository pattern' common in C# and I'd guess Java. Just because CRUD on types is all similar behaviour you can't really build a generic/abstracted way of doing it, because at some point you need different behaviour in some update/insert depending on how the domain concept should act and then you're in a world of pain with a crappy abstraction.
One's code actually 'doing something' isn't a sign of unclean code, it's why one writes it. Stop trying to build abstractions that simplify the doing of what your code does.
But thinking back and considering the intuition I built around when an abstraction makes sense and when it's just going to be unhelpful cruft, boils down to this.
Abstracting over behavior gets messy very quickly. Special cases will probably arise on the next requirement change. Unless they're already there and you missed the subtle interaction. And if you didn't, and actually handled that properly, your abstraction will have a lot of hooks and bells and whistles to support the different behaviors, and it'll just be hell to maintain.
Data can change underneath you, sure, but I think we as humans have a much better intuition about concrete things rather than algorithms, so it's easier to abstract data in a useful way. Which is why I think that works better.
Especially in a CRUD situation, every custom procedure will probably have a few basic steps that stay the same. All you usually need is a pre and a post hook.
It's tempting to think that for e.g. a CRUD situation, pre- and post-hooks are all you need.
And it may be that way at the beginning, although hardly so, except for the simplest applications. But very soon, you start to run into things like (not an exhaustive list, but all are things I actually encountered while trying to do precisely what you're saying, many years ago):
* Transactions, when multiple things have to happen atomically. Your pre-hook must start a transaction, and your post-hook must commit it. But what if there's an error somewhere? Python and JS don't have RAII, so you need some kind of catch block to abort the transaction. Where does that happen?
* Tricky validation, e.g. needing to do queries against the data store to check the request is well-formed. So if you're using an event-based language, your pre-hook also needs to be asynchronous.
* Data transformations (what the user sends will hardly be what needs to end up in your data store), so your pre-hook needs to be able to return a new object.
* Tracing across service calls, so you need to pass some kind of request ID as well to your hooks.
* An "update" request needs to return something (e.g. an ID) to avoid another round trip. But sometimes it needs to return more stuff, so your post hook must be able to return data. Oh, but your ORM or DB usually returns the created object, so you'd like to use that instead of re-fetching in your post-hook, so now your post-hook must accept that as well.
These just keep coming up. The first three in particular are usually guaranteed to happen before the first release of the product, since requirements always change. Soon you have an unmaintainable monstrosity. A little copy-paste is tame in comparison.
[1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Ad...
And the proposed solutions of hooks, AOP etc all start down the road of scattering actual logic across files and methods making the code harder to reason about which seems to be the worst part of overeager abstraction.
The library Automapper from C# is another place I run into this a lot. At its core it simply maps from domain to dto properties but you soon end up needing awful unwieldy configs and magic. I'd rather have a few hundred lines of obvious mapping than ever work with automapper again.
leave the objects alone, but write pure helper functions that implement the most common expressions inside the mathy parts, and have all the similar-looking objects use those helper functions.
0. Existing code is probably working fine, and making changes (for any reason) risks adding a bug. Worse, reviewing code with a bunch of reformatting is tedious and the reviewer may assume you’ve only done reformatting and not notice the accidentally-changed behavior either.
1. You have to merge, a lot. And “fixes” that do little except reformat or redo are a pain to deal with when all you really care about is getting functionality in. This multiplies across branches and team members, and might require multiple manual merges.
2. There is a decent chance the code you’re not pleased with will be ripped out entirely as part of some bigger change, at which point all effort to fix unclean parts is moot.
3. Far more people than you are familiar with how it used to be, warts and all. The “cleaned” version now looks alien to everyone else and might slow them down.
That said, there is a time to clean things up; it just has to be at a well-defined point in the project. It involves a combination of things from the article (e.g. talk to the team) and the above (e.g. do it when many features are merged in and there are few branches to deal with).
Cleaning code is often wasted effort when you don’t have the full picture or understand future variations. Like you mentioned with ripping out your refactor later.
This improves the speed of implementation one and two because we don’t have to think as much. Then third iteration is faster as well because it’s obvious how to abstract.
I’ve worked at “blame game” companies before, if your name was on the commit it was your fault, all other process steps be damned.
So it reduced my inventive to change things unless actually required to ship a feature or fix.
I don't think assuming people who disagree with you lack confidence and are "compensating" is an effective way to reach an audience.
Rewriting your teammate’s code
There should be no such thing as "your teammates code", there is only your team's code. If the changes were improvements they should be welcomed by the team, if they worsen the code they should be unwelcomed - independent of who first authored the lines.
There is absolutely judgement involved in refactoring, code replication, and choosing the right abstraction. But this article doesn't offer much wisdom that helps with those decisions.
This one was a bad example. But past 5 or so years of experience, when someone has experience with other languages and coding styles, it's more interesting and I often learn a lot from them.
Change for change's sake may be bad for team dynamics. But if the change is one that everyone agrees is for the better, no one should be offended by the improvement.
I had programers on lousy toolchains argue for the 9000 codeline one-file copy paste monolith, because it was "objectively" easier to debug.
There is always a teammates code, who will be different.
> I didn’t talk to the person who wrote it. I rewrote the code and checked it in without their input. Even if it was an improvement (which I don’t believe anymore), this is a terrible way to go about it. A healthy engineering team is constantly building trust. Rewriting your teammate’s code without a discussion is a huge blow to your ability to effectively collaborate on a codebase together.
Also (2020).
If I see a code I want to change, I do it. I don't ask for permission. And colleagues can do the same with code I wrote. We trust each other.
There's plenty of reasons to not engage with someone. There's also plenty of reasons why it may make sense, and I think it's highly context dependent. For me, it's about 50/50. And... in the cases where I reach out, it's about 50/50 as to whether they have any time/inclination/memory/ability to help anyway.
A brief chat can then help to clarify things.
Realistically, people doing such work care very little about writing 'great code' because they know they have no real 'ownership'. There hopes for higher pay and recognition rely on climbing the corporate ladder by whatever means available. Team member, team leader, division manager, VP of whatever, etc. Blame bad results on someone else, that's the normal tactic for these types. Don't hold up production over code quality concerns, because delays in pushing product to market upset the shareholder board, which they see as lost profits.
The whole notion of a 'skilled technical individual who takes pride in their work because they own it' sounds like some awful corporate in-house propaganda campaign to be honest. And this accounts for much of the current mass exodus from the corporate workforce, I imagine.
The code I write is corporately owned in the legal sense but I still feel some attachment to it and care about good workmanship.
I'd ask first if they agree with a certain improvement. It shows I value them as an engineer and they might bring up critical information that I'm missing, making my "improvement" actually worse. It doesn't hurt to talk to people.
"This fence is in the way and obstructing the flow, it should be removed"
The teacher said to the student:
"If you can tell my why someone made the fence in the first place, I will allow you to remove it."
Note that this has nothing to do with "code ownership". If you worked for me and randomly changed code that you did not like, I would fire you.
From one extreme to the other? People make mistakes, educate them instead of brutally punishing them. Talk it through - isn't this exactly the mistake that was made by the developer (they didn't talk to their peers)? If they do not respond to feedback, that can eventually lead to a firing.
However, if the behavior persisted then yes, I would fire them.
If anyone sees code that they hate they are free to submit an issue and I will be happy to review it and prioritize it with the other work to be done.
If they hate code that smells so much that they want to volunteer their time to refactor it, then they can participate in code reviews for me instead.
Ha ha ha. That's not how it works with me.
This was all a bespoke CRM system. I poked in the code (it was something I had access to) and noticed that ... the entirety of the whole screen was duped - the output of the client's history was embedded in an HTML comment tag. So... if the client had, say, 3 years of info/comments, it was rendered, then rendered again in HTML comments. It was a single line duplicating the entirety of the info. It was obviously a debug remnant. I removed it. I tried to talk to the original developer beforehand - he was on the phone, and kept waving me off. I emailed him. No answer. I committed it and got it to a testing server. The test guy immediately loved the speed improvement, and we got it out to the floor. There were about ... 50-70 agents live at any one time, and everyone's hold/wait times were cut by 20-30% overnight. Clients and agents were happier. I found out later that, over time, we cut bandwidth bills by a measurable amount.
I was raked over the coals and nearly fired for that. "unprofessional", "insulting", etc. It was primarily because I'd made someone else look bad. Other people on the team also made changes now and then to each others' code without asking permission, when it was needed/obvious (emergencies/etc). It was a simple oversight in removing some debug code, and it made a huge difference (it had been tested and had other eyes on it as well - this wasn't "change prod by hand without telling anyone").
Still bugs me to this day (as you can tell).
"My code traded the ability to change requirements for reduced duplication, and it was not a good trade. For example, we later needed many special cases and behaviors for different handles on different shapes."
The reason for my choice of this point is that provides a direct, technical counter to one of the simplistic technical rules on which the Clean Code movement is founded, and so might make a impression on someone who has, heretofore, regarded the case made by Clean Code proponents as irrefutable.
That's also why I say "in this particular context", as in the broader scope of software development practices generally, the interpersonal dynamics of the organization are almost always more important than coding style.
Probably not the best time to refactor someone else's code that currently works.
Acest articol este interesant
Math code also seems way more amenable to being consumed as passed-in functions that are consumed as a black box, then those things call the math functions and get the result, and really don't care how the math was done. But it's then good to signal (debug logging) what math function was passed in and why. Here, you can have OvalSpecialCaseMathFunction() along side the regular ones too.
I'm probably falling into the same pit as the author here, but it doesn't seem like duplicating every "kind" of code has the same cost/benefit.
It's not just about reducing lines of code and increasing abstraction, which I agree are bad metrics. It's about legibility. He took a block of 140 lines of dense math descriptions of objects, and reduced it to nicely structured semantic code. It's a 100% improvement, if that code has a bug, anyone could read it and find it.
The author said they would later need support for different behaviour that would have made his code more convoluted. Well big deal, it's not like there was a way to do it without violating open/closed anyway. This code is so easy to understand, anyone can read it and go "ah this won't work with the new requirements, let's delete it and start over".
Not saying this code 100% needed to be refactored, but if you're an engineer and you see it and you think you have some spare energy to pretty up a piece of the codebase, and you diligently test and verify your implementation, why not do it?
I wish my team would do this more often.
I really appreciate it, when people write from hard-won personal experience.
For myself, I write in Swift, and it’s quite possible to write totally inscrutable code in Swift. I am guilty of making code harder to read, and less grokkable, in my “cleanup” sweeps.
I am currently doing a bit of navel-gazing, on this very topic. I have gotten into the habit of writing very “swifty” code, and am thinking that I should probably back off a bit from that.
My current codebase has a lot of duplication for fairly trival things, because the previous codebase tried generic solutions, only to end up needing a lot of exceptions, which ended up making the code look messy and hard to maintain.
Not a single word about what the boss objected to. Reading between the lines, I suspect the author still may not know. That's the problem, not Clean Code.
A lot of bad refactoring is sort of like putting Chair as a subclass of Dog because both have four legs. Or someone thinking that long paragraphs have high cognitive load, so all paragraphs are split into 2-3 lines.
In this scenario, OP simply changed the code without understanding what problem it was meant to solve. It's not about trading readability for flexibility, or respect or anything. It's that the code was probably further from solving the problem.
All around a mess. I agree with the sentiment (write clean code unless you shouldn’t) but there is some serious process issues here that should be caught and corrected and would have made this entire issue something during the PR review phase, where OP presumably asks for an improvement and is answered with the context for the decision. Not to mention the red flag of “I know better than my peers” attitude of many programmers I have known in the past, leading to distrust and backstabbing. It can get ugly.
There's also the fact that with complex abstraction, you run into the problem that it can be more difficult to maintain as there's more layers of indirection.
That cost of abstraction has been mentioned elsewhere, but I want to throw out there that both of these points fall under the umbrella of a powerful concept for writing maintainable code, that you should not only think about the best way to structure your code now, but you should consider how that code will change and how that structure will facilitate or hinder that.
What I think I would have favored in this case would be to use some small, well-named utility functions that can be used to implement that math (not necessarily to replace all of it). I cannot see the code so I don't know for certain if utility functions would actually help, but I have taken this approach for geometry oriented code in the past and it worked well. In general, don't be afraid of writing small utility functions.
I don't agree with the concept of letting "clean code" go. All he's done is replace "clean code" patterns with other ones that work better for the situation. What's best is becoming familiar with new patterns like the ones I've described above, not just "clean code" patterns, weighing their benefits and downsides for the current case, and not necessarily worry about whether you're using patterns that are arbitrarily part of a paradigm that hasn't worked in other cases (however, remaining aware that you don't know every pattern).
Of course, that's looks like a weak definition, because it ends up falling to, "how do you define your use-cases?" Well, that's kind of the point. The biggest thing to point out is that use-cases are not static, but change as the business changes. The second point exemplifies this:
For example, we later needed many special cases and behaviors for different handles on different shapes.
So now they're definitely separate use-cases, that just happen to look similar because they're achieving similar goals. But the methodology may and can be completely different depending on the circumstance.
So even if this was originally same code, and the unification was successful, it would have ended up split apart again anyway. And that's OK to. Sometimes things that really are the same use-case end up splitting as the business changes or learns. We as engineers need to recognize when this happens and split out the code also, instead of creating a tangled mess of one code serving two different use-cases.
EDIT: Just read some of the comments, and my thoughts expressed here are probably a subset or incomplete version of "prefer duplication over the wrong abstraction":
Repetition isn't necessarily a bad thing. If the repetition isn't difficult to alter and it more clearly describes what is happening on a step by step basis, it's difficult for me to call that "dirty".
Code is for humans, not computers, first and foremost. Certain things necessitate performance early on, but for a lot of projects it isn't reasonable for all code to make absolute logical sense from the get-go. What's more important is that people can read the code and understand it without undue deciphering.
Sometimes I come back to code that I've written long ago. When I wrote that code, I almost always thought I was writing "clean code". It often turns out that the code I was immediately able to understand and make changes to was the code that had repetition, wasn't mindlessly spread out into a bunch of "tiny functions", and had comments spread out to describe my reasoning.
In contrast, "clean" code is often inherently hard to change because it relies on centralizing functionality. When you centralize something and you change it, it may work for one circumstance but mysteriously cause something else to break. With repetition, your code base might be larger but code may be more decoupled and thus easier to make a small change to fix one thing without breaking another similar thing.
Flexibility should be kept in mind. In the blog post example I would have at least abstracted the math out and there's no reason of the limitation of one handle function, you could swap them out.
Yeah I may a be a bit OCD, no I'm not trying to be clever. I'm trying to reduce complexity so I can keep the entire system in my head to optimize things easier. Don't atomize everything into tiny functions though, there's a huge middle ground.
No book gets everything right, but I recommend The Pragmatic Programmer for this sort of thing.
People seem to use wrong instances of people "cleaning code" to justify dirty code and tech debt.
Using a different form of deduplicating would be the solution here. Just write a few functions that do the math and then call them, rather than create new abstractions that can later become monstrosities. I think they learned the wrong lesson here.
Just my two cents :)
Say you have (a*b+c)*2.0f+d several places in your code with potentially different a b c d variables. Are you really going to make up a new name for it as a function? It's much easier to just read the expression to know what it does than to memorize a new name for every possible small occasionally duplicated expression.
I've been a bit of an apologist.
I thought maybe people just took clean code a bit too far, that people were being too dogmatic about what was basically sound advice. Then I found Uncle Bob's old website the other night and I can no longer deny he's had a finger in this.
> _Anything not testable is useless._
> Here's the thing. If you can't test it, you don't know that it works. If you don't know that it works, then it's useless. If you have a requirement and cannot prove that you have met it, then it is not a requirement.
- http://www.butunclebob.com/ArticleS.UncleBob.AgilePeopleStil...
If these are the vibes you're putting out, your followers are going to be zealots. They're going to be frothing at the mouth whenever someone proposes alternative paradigms.
It requires some massive balls to deploy code to production while not even seeing if it works for any case.
If the answer to "how do you know it works?" is "trust my judgement bro" then I will not, in fact, trust their judgement.
I do agree he has a finger in the zealotry and he should probably have modulated how he expressed himself.
Just because Uncle Bob, seller of unit testing software and TDD-books will hear of no other doesn't mean that this is the One True Way.
I haven't actually read his site, I'm going by what was quoted.
Might be that he meant "unit tested" when he said "tested", I guess.
The buzzwords are like catnip to most developers. They go absolutely crazy. Their fevered madness, detailed on medium.com. Their codebase, transformed. Conference tickets, bought. Once they've weaned themselves off one then another buzzword will come along and books will be bought and blog posts will be written and code will be refactored and job specs will be altered.
I live by "when you have a hammer, everything looks like a nail" now and try to stay wary of this year's transformative codebase panacea.
Firstly, was there any kind of review or just a chance for team members to talk about the original implementation before it got onto master? After the fact?
Second, Ok, the master commits seem liberal, but why then the boss goes the one-on-one way to make a team-member purge the commit? It seem like a wasted chance at regreasing the team dynamic, let developers find a way to forge the code together.
There should be an open channel between devs and also a way for them to express themselves by means of code... without fear of blame or dangers to the mainline. 'Branches are cheap', isn't it? Personal-branch it so it could later be showcased to others, to the boss?
IMO, this is more about Open Team, than Clean Code.
Yeah code duplication is fine when the commonality of code is incidental. The thing that bothers me is starting from the assumption that the requirements for each shapes are going to diverge. It looks like the wrong kind of premature optimization to me. In this case the refactoring seemed sensible, and unless they already _knew_ that this would happen soon, I think it was safe to assume that it wouldn't and that factorizing made sense. _Then_ duplicate the code in the future if it's necessary - seeing that the abstractions were not working properly.
There are tons of codebases in the wild with large portions of duplicated logic, from experience they seem the rule rather than the exception for any sizeable project. "DRY" as itself is not a rule, but I think it makes sense to be doubtful of duplication as a default stance.
Also of note is the functions with an if statement at line 1 so it's basically 2 functions with 1 signiture because it started dry and then requirements changed. But hey, at least it's dry right?
Learning that you can deduplicate code.
Learning that deduplication causes coupling.
Learning how to inject behaviors.
Learning how to discuss problems before job hobbying someone else's code.
Learning that all code is really not what the end user cares about. They only care that it works and corporately you want to do it as cheaply as possible, privately you want to remain employable and increase your marketable skills. Everything else is a distraction.
What is described as building trust can be also interpreted as groupthink and colleagues forming pacts of mutual non-criticism (which breeds mediocrity). Which is also the way to build dogmatic cults of personality, and becoming superficial developers that care more about talking about their weekend than their craft.
You do not trust code. Memory and attention are fragile. Everyone makes mistakes. Everyone can have a bad day, be distracted, tired, etc. Building trust sucks. Trust nothing and be able to verify everything.
Your job is to edit code: add, delete, modify code. Each time you check in code it becomes company property. It is not "your" code. Modifying other people's code is completely OK, and expected.
In the end, this article is just wrong. "Give up, tech debt is your friend. Your job is to be best friends forever with your colleagues". All wrong conclusions.
1. It seems to me that you refactored the code in the wrong dimension. I would have abstracted out a template shape object.
2. Are you not using a pull request/peer review process to accept changes into the prod branch?
3. I don't think you quite arrived at the right conclusion. While you conceed that your original conclusion was incorrect, I do think your original criticism has merit, and while your solution may provide a different set of problems, that doesn't mean that a different solution based upon the collaboration between yourself and your colleague might have been yet even better.
In the real world of real people coding and not in click-bait land, people use clean code as a tool that's sometimes appropriate.
2. A master follows the rules because he understands the point of the rules.
3. A guru breaks the rules because he knows they don't apply.
It’s not duplication at this point.
edit: I’m an idiot. Perhaps not making it an abstract class. But it’s one of those examples where inheritance might actually help.
Sometimes duplicate code is simpler.
Sounds like he did a lot more than eliminate duplication, and perhaps that's what his boss was unhappy about.
REEEEEEEEEEEEEEEEEEEEEEE!!!!!!!!
/s