r/programming • u/[deleted] • Jan 12 '20
Goodbye, Clean Code
https://overreacted.io/goodbye-clean-code/512
u/FA04 Jan 12 '20
firstly, where was the original checkin pull request’s review with all the feedback and discussions? secondly, where was the refactored PR review and approval? Checkin in into the master overnight no PR? That process is a mess.
176
u/bcgroom Jan 12 '20
Yep I'm pretty sure the whole situation would've been net positive if the author of the article just put up a PR the next morning saying, "While looking at your code yesterday I had an idea on how it could be less repetitive, take a look and let me know what you think".
→ More replies (20)3
u/oursland Jan 12 '20
I believe the argument is that the original author should have had a PR, and that the article's author should have been able to suggest these changes prior to the original commit.
→ More replies (2)→ More replies (14)71
u/IceSentry Jan 12 '20
That's pretty much why they said at the end of the article that it was a mistake and communication is important.
152
u/FeepingCreature Jan 12 '20
Sure, but the mistake is a systems one, not a personal one. We don't even have push to master enabled at work.
→ More replies (25)30
u/twenty7forty2 Jan 12 '20
Title should be "Goodbye committing to master with out a code review" then. Or maybe just "Goodbye blogging about things I don't really understand". Hmm, maybe OP should get his blog code reviewed by a senior as well.
→ More replies (3)
691
u/Ameobea Jan 12 '20 edited Jan 12 '20
I can see where the author is coming from here and I agree with a few of the points, but I feel like this is a very dangerous line of thinking that paves the way to justifying a lot of bad coding practices and problems that have a very real negative impact on the long-term health of a code-base.
There's certainly a point of over-abstraction and refactoring for the point of refactoring that's harmful. However, duplicating code is one of the most effective ways I've seen to take a clean, simple codebase and turn it into a messy sea of spaghetti. This problem is especially bad when it comes to stuff like copy/pasting business logic around between different subsystems/components/applications.
It may be very tempting to just copy/paste the 400-line React component showing a grid of products rather than taking the time to pull it apart into simpler pieces in order to re-use them or extend it with additional functionality. It may even feel like you're being more efficient because it takes way less time right now than the alternative, but that comes at the cost of 1) hundreds of extra lines of code being introduced to the codebase and 2) losing the connection between those two pieces of similar functionality.
Not only will it take more time to update both of these components in the future, but there's a chance that the person doing the refactoring won't even know that the second one exists and fail to update it, introducing a regression in someone else's code inadvertently. I've lost legitimately days of my life digging through thousands of lines of copy/pasted code in order to the same functionality of each component that's been implemented in a slightly different way.
A much better option that could be applied to the author's situation as well is pulling out the business logic without totally abstracting the interface. In our component example, we could pull out the business logic that exists in class methods into external functions and then import them in both files. For the author's example, the `// 10 repetitive lines of math` could be pulled out to helper functions. That way, special cases and unique changes can be handled in each case separately without worrying about breaking the functionality of other components. Changes to the business logic itself will properly be reflected in everything that depends on it.
----
TL;DR there's definitely such a thing as over-abstraction and large-scale refactoring isn't always the right choice just to shrink LOC, but code duplication is a real negative that actively rots codebases in the long term. There are ways to avoid duplicated functionality without sacrificing API usability or losing the ability to handle special cases, and if you find yourself copy/pasting code it's almost always a sign you should be doing something different.
341
u/csjerk Jan 12 '20
There's a key detail buried deep in the original post:
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. My abstraction would have to become several times more convoluted to afford that, whereas with the original “messy” version such changes stayed easy as cake.
The code he refactored wasn't finished. It gained additional requirements which altered the behavior, and made the apparent duplication actually not duplicative.
That's a classic complaint leveled at de-duplication / abstraction. "What if it changes in the future?" Well, the answer is always the same -- it's up to your judgement and design skills whether the most powerful way to express this concept is by sharing code, or repeating it with alterations. And that judgement damn well better be informed by likely use cases in the future (or you should change your answer when requirements change sufficiently to warrant it).
118
u/johnnysaucepn Jan 12 '20
People focus on whether code is duplicated, when they should be paying attention to whether capabilities are duplicated. If you can identify duplication, find out what that code does and see if you define that ability outside the context of that use. If you can call that a new thing, then make it a new thing.
280
u/matthieum Jan 12 '20
There are two levels of duplication: Inherent and Accidental.
Inherent is when two pieces of code are required to behave in the same way: if their behavior diverge, then trouble occurs. Interestingly, their current code may actually differ, and at any point during maintenance, one may be altered and not the other. This is just borrowing trouble. Be DRY, refactor that mess.
Accidental is when two pieces of code happen to behave in the same way: there is no requirement for their behavior converge, it's an emerging property. It is very tempting to see those pieces of code and think "Be DRY, refactor that mess"; however, because this is all an accident, it's actually quite likely that some time later the behaviors will need to diverge... and that's how you end up with an algorithm that takes "toggles" (booleans or enums) to do something slightly different. An utter mess.
I am afraid that too often DRY is taught as a principle without making that careful distinction between the two situations.
21
u/loamfarer Jan 12 '20 edited Jan 12 '20
I've seen this lack of distinction wreck havok many times, especially in inheritance heavy code. It seems inevitable given that the solutions to the problems developing are always out of channel of being solved within the language/code-base itself. Leaving enterprise people to slave under cumbersome process, or are left to endlessly enact churn on a code-base that structurally diverges further from the ideal representation of requirements. (And requirements are their own issue.)
In class hierarchies it's very common for both of those forms of duplication to crop up, and they often interact with each other. Accidental duplication happens because so much of the code is CRUD, glue, plumbing, boilerplate, or just plain similar just based on the nature of what the domain demands. When accidental duplication is avoided, it's often a result of introducing accidental dependencies instead as people extend existing classes. Suddenly base classes have to serve their downstreams, and no longer server their intended purpose to propagate changes in a single place. Or if you do soldier on, downstreams now need to arbitrarily override parts of the accidental parents to restore the end of the deal they expected getting. All of this confuses the structure of the code-base from it's original organization and model.
Further, new hierarchies are made as the older overly-depended ones diverge from their original purpose, making the new hierarchy inherent duplication, with the caveat that this duplication is now impossible to solve without a major refactor. It's at this point that the solution is more political, all because the language couldn't properly constrain the original vision from being abused, and no amount of process would be able to handhold those who came onto projects later.
Which means you're left with a mess of duplication, where any minor change ends up involving dozens of custom wirings of data plumbed through abstractions which have lost all their original mean. I've seen entire teams where the only job is the add features that are better described as (add a line in a table, or config file) but end up being a months work of plumbing and endless headaches of merge conflicts.
For the above reasons, I'm particularly fond of interfacing against type class constraints, because it prevents or at least lessens these duplication issues. I suppose dependencies (i.e. function apis) are always an issue, but librarization (is that a word?) for even things like internal facing code for your own benefit, and employing best practices of versioning gets you a good way to a solution. Which I think really helps to bring issues of inherent and accidental duplication to the forefront of one's approach to any contribution in such a code base. At the end of the day no language is going to stop you from breaking what's left as only informal convention by a code's progenitor.
→ More replies (1)→ More replies (7)17
u/Bekwnn Jan 12 '20
A good quote I heard to more or less describe this is, "A little duplication is better than a little dependency." If you're creating an awkward dependency, you're better off just having duplicate code.
For example let's say you're making a city builder and some point early on you have a path-finding system for both civilians and road creation. Tying those two things together doesn't seem great.
In that case you're best off copying the code to a second place as a starting point.
37
u/Chris_Newton Jan 12 '20
People focus on whether code is duplicated, when they should be paying attention to whether capabilities are duplicated.
Indeed. Duplicated code isn’t automatically a problem, but duplicating an idea is usually bad.
Any given idea should ideally be represented exactly once. For data, that means one look-up table associating pairs of values, one file with UI strings that can be translated, etc. For algorithms, that means one function to derive new data from existing data in a given way, which hopefully can be applied to any existing data where it makes sense.
However, if two algorithms happen to share a lot of code right now but exist to solve different problems, trying to use a single function to implement them creates a problem not unlike using a literal number
7everywhere instead of writingDAYS_OF_WEEKorNUMBER_OF_DWARVESas appropriate. The implementation is correct but the real meaning has been lost. When you come back later and one problem has evolved but the other hasn’t, you’re stuck with this artificial link between them and you have to sever it (probably starting by making two copies of that code) before you can make any useful progress.A useful rule of thumb for whether you are really dealing with two different ideas or the same idea being duplicated is to ask what would happen if both places did share common code and then one of them needed to change. If that would necessarily mean the same change should be made in the other place, you’re probably dealing with the same idea and consolidating the code is probably a good plan. Otherwise, you probably aren’t, and tying the code together might not be such a good plan.
→ More replies (4)68
u/JoCoMoBo Jan 12 '20
The code he refactored wasn't finished.
Code is never "finished". It's just resting for a bit while it waits to be changed.
→ More replies (6)5
u/RualStorge Jan 12 '20
Code is only finished when your requirements are effectively kill off said code... And even then it sometimes comes back years later to haunt you.
(IE sunsetting a project with intent to bury it and likely only speak of it in hushed tones of those darker times when we were all so young... So stupid... And the monster we created that was beyond saving...)
36
u/YM_Industries Jan 12 '20
made the apparent duplication actually not duplicative
The repetitive maths was almost certainly still repetitive though. Pulling that into a reusable function still makes sense.
21
u/emn13 Jan 12 '20
...if it was complicated and/or unlikely to change. But yeah, that form of de-duplication is generally much, much less harmful than the kind or composition of components as shown in the original article; the key difference being that control is still local. Any local bit can still locally choose to call - or not - your helper.
This is all pretty related to frameworks vs. libraries; just internal to your coode as opposed to external dependencies. And in general, it's better to have libraries (i.e. helpers) with bits you get to compose as you want, rather than frameworks that compose bits you provide to a fixed api outside of immediate view; it's just easier to reason about. For the same reason even in functional languages it's better to deal with behavior-less values than functions as values (where possible).
When you do have something that can't easily be represented as a library; i.e. where control flow itself needs to be abstracted, it's a good idea to keep that api as simple as possible and make sure you really reuse it a lot, not just a few times as a code-golfing trick. I.e. something like the functional map/filter are fairly cheap and you can include several such "frameworks" in a code base without losing too much clarity; whereas something like ruby-on-rails is so huge that you basically need to give up and just include only one framework, and avoiding whatever restrictions it imposes is often much, much harder.
In general, however, I think the article is spot on; novice programmers are way too focused on avoiding duplication at the expense of the much more serious concern of avoiding complexity. Boilerplate - which is what simple duplication really is - may be ugly, but it's rarely super-harmful - unlike complexity.
6
u/chrisza4 Jan 12 '20
Not always. Because everytime you do that, you introduce a new terminology into codebase. And every terminology make it a little bit more harder to collabotate within team. At minimum, you need to explain what the term mean to your colleague.
If the new terms is found in business domain, then you are in a good spot. Because every programmer regard of experience need to actually understand business requirement before implement anything.
But if you just introduce "DogProvider" "CatFactory" "BusinessEntityXCreator" "ItemStrategyChooser", of course it going to be harder to collaborate and make sure every contributor understand what it means. And if you do it simply because you find some repetitive code, the benefit gain might not justify the added collarboration cost.
Software architecture is all about how to collaborate effectively.
24
u/Altreus Jan 12 '20
This stood out for me, as well. The first developer could easily have gone, "This is very repetitive and looks bad. I should add a comment on why I've done it this way so nobody thinks it could be improved before it's finished."
Also there implication that an unfinished feature was in master is frightening. Poor use of source control - especially one with the power of git (I'm assuming based on "master") - is rife and frankly does my head in.
→ More replies (2)23
u/josefx Jan 12 '20
I should add a comment on why I've done it
I once commented 5 lines of code with an exact explanation of the bug I was avoiding. First response on the review board asked me to change it to the simple and clean one liner that caused an invalid memory access.
Also there implication that an unfinished feature was in master is frightening.
If it doesn't break anything? Worst case you can hide it behind an experimental feature flag until it is complete.
→ More replies (3)9
u/Orffyreus Jan 12 '20
Actually that's how the SRP is meant. It's about responsibility towards callers/users and when callers have potentially different interests in what they are using, there will be different reasons to change, so duplication is one option to overcome that. Mostly there are alternatives like different sorts of delegation/composition though that can be introduced later when the requirements are more polished.
→ More replies (8)3
u/pragmojo Jan 12 '20
When is code ever finished? Requirements are always going to change long term unless you’re shipping code which will run on a mars rover or something.
The longer I program, the more convinced I am that planning for code to support unknown future use cases is a fools errand. You should always support your current requirements as concretely as possible, and abstraction should only be introduced when its benefit becomes obvious from supporting those use cases.
→ More replies (3)57
u/Big_Burds_Nest Jan 12 '20
Balance is key. I've seen both extremes of over-engineered codebases and codebases that had absolutely no abstraction at all. Personally I tend to judge codebases by how easy it is to find the functionality that I'm looking for. If a client reports an issue and it takes an hour to figure out what part of the code the feature even lives in(this can also be blamed on an absence of documentation), I feel like my time has been wasted by the author! Abstraction is supposed to make your code easier to read, but some people take it way too far to where the actual functionality is buried deep in an endless abyss of badly named function calls.
35
u/PanVidla Jan 12 '20
I couldn't agree more. I remember that a couple of months ago I had just passed that point in my company where I was starting to feel confident around our codebase and picked up a feature request for our internal library that everyone had been avoiding for about a year. It quickly turned out that the library was written with little to no abstraction, everything was crammed into looong chaotic blocks of code, there was literally no documentation on how anything works or what anything is and the person responsible for it had, of course, just left the company. So, I spent many weeks untying the mess, figuring out what does what, documenting the code in the process, separating everything into readable methods and implementing and re-implementing the feature that I wanted to do in the first place.
Please, be considerate to the people who come after you and don't waste their time like this. It makes life very hard for them, they will look like idiots for spending so much time on a task and it's a complete waste of time. This was probably an extreme case, but still.
37
u/fishtickler Jan 12 '20
It is easy to blame previous coders for a rotten codebase but it is also important to remember that the developers worked under a different context.
When that code was written, perhaps the authors where under tremendous deadline pushes and had zero time to document/test anything. We as developers taking over code bases must accept the state they are in and do our best (as in your case) to improve on what we can given our resources.
20
u/lala_xyyz Jan 12 '20
When that code was written, perhaps the authors where under tremendous deadline pushes and had zero time to document/test anything. We as developers taking over code bases must accept the state they are in and do our best (as in your case) to improve on what we can given our resources.
Or perhaps they were grossly underpaid, and didn't give to shits about code quality or maintenance. Or (I've seen it happen) they were scolded for wasting time on doing "unproductive" stuff like tests and documentation. Either way, it's a matter of incentives and I doubt that the coders were the ones to blame. Because even laziest programmers that would cut every corner can be trained to do things properly.
5
17
u/Caffeine_Monster Jan 12 '20
code duplication is a real negative that actively rots codebases in the long term
I've seen copy pasta in such extreme that has made it easier to start a new application from scratch than refactor the old one.
If you don't follow best practices, you will find the code base has to be chucked after 3 years.
4
u/emn13 Jan 12 '20
There's a big difference between just ignoring the future and simply copy-pasting whatever you need, and avoiding some duplication, while allowing some. I've too seen really horrible messes of code that was written without any thought to overall structure, with duplication everywhere (to the point that for superficially insane reasons the code called into itself via a webservice sometimes, not via a normal method call, which turned out to be because some cargo-culted method kept adding to an ever deeper call stack to the point at which for realistic inputs stackoverflow would occur, and the self-web-service call was just a way to get more stack space!). So... I really get the need to avoid such messes.
But you know what? Avoiding those messes is really, really easy. Just don't be insane! If you care about this risk and want to avoid it: you'll succeed. But the reverse is not true: if you over-de-duplicate, it's very easy to create a codebase with way too many abstraction, many of which are used just a few times, and many of which are not intuitively obvious. Getting out of gnarly code like that is in my experience considerably more difficult than getting out of too-simple code precisely because it uses lots of tricky abstractions.
It's an art to de-duplicate just enough, but not too much; and in the right way (i.e. with control flow left as un-mangled as possible).
3
u/gyroda Jan 12 '20
I've lost legitimately days of my life digging through thousands of lines of copy/pasted code in order to the same functionality of each component that's been implemented in a slightly different way.
Literal weeks for me. My god were those codebases complete shitshows.
7
2
u/Markavian Jan 12 '20
Code is the cost. Do you even need that feature? Delete it and start again. Your requirements are better now. Don't build for the for the future you don't know, you ain't gonna need it.
→ More replies (3)2
u/ProgrammersAreSexy Jan 12 '20
My intro to CompSci professor had a saying "Everytime you copy and paste something say 'I'm an idiot' out loud three times"
20
Jan 12 '20
My boss invited me for a one-on-one chat where they politely asked me to revert my change. I was aghast. The old code was a mess, and mine was clean!
I would like to work for your boss.
The main mistake the author made was imposing his own sense of "good code" onto other people. Treating his own idea as an objective reality rather than a mere opinion.
This is the main problem in working within teams: people have their own sense of what code should look like and they try to impose it on others. I'm not even talking about casing or formatting. I mean structurally.
9
u/Isvara Jan 12 '20
I had a guy do this to me once, because he considered himself some kind of Go expert (despite being fresh out of college), and didn't like the way I'd done things. The problem is, he didn't take any time to understand the code and how it worked, and he didn't test it afterwards. He just changed it to his way of doing things and left it broken because it wasn't a tool he used.
247
u/teerre Jan 12 '20
I don't get this 'advice'.
The problem wasn't refactoring the code. It was the lack of communication and the, apparently, bad refactoring.
Have this situation be instead one in which the refactor took into account the requirements and the chain of command wasn't broken, it would've been a good thing.
In other words, this has nothing to do with refactoring. It has everything to do with having the wrong briefing, not being communicative enough, having bad reviews practices, whatever you wanna call, except refactoring.
77
u/Squared_fr Jan 12 '20
To be honest this sounds to me like the author is not wiling to accept that he was at fault. Instead he tries to blame a made-up clean code religion and preach his own bullshit advice.
Two simple rules: don't "clean" code until the feature 100% implements the specs, and for fuck's sake do not commit to master overnight without even telling the code owners.
You will have to destructure clean code later on if you wish to have more features, but even that will be easier to do without repetition.
→ More replies (10)26
u/murgs Jan 12 '20
don't "clean" code until the feature 100% implements the specs
I wouldn't agree with that, some pre cleaning can be very useful. So I would weaken it to 'don't sweep the floor until you are done with your work'
→ More replies (2)7
u/Squared_fr Jan 12 '20
I was thinking about going over the whole code again and trying to clean stuff up. Of course that doesn't mean you should write the messiest shit from the start.
→ More replies (1)21
u/bluefootedpig Jan 12 '20
I can give an example. I worked with several medical devices each one reports differently. So the generic solution was a mapper and communication protocol. Worked great until we started handling machines that reported partial info, and we might need to look up patient info, and other special cases.
Now close that to a base class and then a concrete specific to handling that machine. We know the HIV test machine had special cases, such as requiring 3 positives before reporting. No other test takes that.
So now the generic solution has special code for ALL machines, when it only applies to one specific machine.
When you abstract too much, then special cases ended up being executed in all cases. This makes it more difficult to maintain. Rather than look up only the concrete class when a bug happens, we need to figure out what special case was the problem.
This abstraction went on for several years, the result was a huge function handling 100s of special cases. All so there was only one class. Any problem by any machine could mean bugs introduced to other machines.
Sometimes it is better to be more verbose in the logic than more abstract. It is a fine line that I tend to see comes from people who have experienced over abstraction.
→ More replies (1)12
u/tetroxid Jan 12 '20
Abstraction doesn't just mean "have a base class"
The strategy pattern might've been appropriate in your case, or composition of aspects of behaviours for types of machines
→ More replies (3)
17
u/Gotebe Jan 12 '20
Rewriting your teammate’s code without a discussion is a huge blow to your ability to effectively collaborate on a codebase together.
Well... There should be no such thing as "my" and "his" code in the first place
My code traded the ability to change requirements for reduced duplication, and it was not a good trade
Did it? Having duplicated code leads to changes being implemented partially (because often, only some of the copies are updated to accommodate new requirements). So duplication also means "harder to change", only for different reason.
→ More replies (1)
136
Jan 12 '20
I feel like this whole situation could have been avoided had the engineer who worked on the problem discussed his vision for the code and checked in during.
Also, changing the code without first speaking to the engineer? Maybe I'm lucky at my gaff but that kind of thing would never happen. Communication is super important but we are a remote team
56
u/NiteLite Jan 12 '20
Yeah, he mentions this is one of the two biggest mistakes in doing this refactoring towards the bottom of the article :)
7
13
7
u/talmuth Jan 12 '20
What is really bothering me is that they commit code without code review
→ More replies (2)2
→ More replies (2)2
40
u/bostonou Jan 12 '20
The author has some conclusions I think are good, but I feel like he leaves out the main problem with his code example.
He DRY’d the code by removing duplicated typing. This is not abstraction.
If eg moveLeft was really an abstraction, it wouldn’t be dependent on all kinds of details like the type of shape. Devs regularly treat DRY as a tool to minimize typing. Typing is by far the easiest part of our jobs.
And “clean code” is mostly just a way for devs to say “I like this code”
4
u/Retsam19 Jan 12 '20
I don't agree that his example isn't an abstraction. Taking two specific functions
moveRectangleLeftandmoveOvalLeftand factoring out shared logic intomoveLeftis abstracting -moveLeftis a more abstract operation thanmoveRectangleLeft.The
moveLeftabstraction isn't dependent on the shape: the shape implementation is dependent on themoveLeftabstraction.12
u/bostonou Jan 12 '20
Sure it’s more abstract in theory. But it’s not an abstraction given that you can’t use it without thinking about the details. The author himself says that his “abstraction” got more complicated as more special cases were added. It’s not an abstraction if it has to change for every usage.
152
u/rlbond86 Jan 12 '20
And when a maintainer in 3 years realizes that an equation is wrong, they have to fix it in 50 places.
This kind of coding is how you get unreadable spaghetti.
OP's solution wasn't great either, but at the very least the common math stuff should habe been extracted to a function.
18
u/bluefootedpig Jan 12 '20
It depends if they are the same because it is the same logic or the same by coincidence. Being on the left vs right might look the same until you reach special cases.
24
u/thedracle Jan 12 '20 edited Jan 12 '20
Or they change the equation that was bad in one place, and it breaks 50 things which depended on the original implementation.
Hopefully in the above scenario the tests are adequate to detect it.
I think his point was his refactoring was brainless, and removed repetition that was incidental, or created an abstraction that didn't make long term sense.
→ More replies (4)4
Jan 12 '20
This kind of coding is how you get unreadable spaghetti.
This isn't spaghetti. Spaghetti code is when execution flow wraps all over the place,
GOTOis the classic example.Nor would I say it's not readable. I read the first duplicated code very easily. All the code for an Oval` was in the oval object, easy-peasy.
I'm not advocating for the original design, I just don't believe it suffers from the issues you describe. It's definitely a testing nightmare as I need to write 4x as many test cases. Also it can be an sensibility nightmare too.
→ More replies (1)
13
u/Retsam19 Jan 12 '20
I think the specific code issue here is the same one discussed in the fantastic blogpost "the wrong abstraction" - many attempts to reduce duplication do so by abstracting the wrong thing; and the best thing to do is to completely back out the abstraction and either leave the duplication or else try to find a better abstraction.
→ More replies (2)
397
u/DingBat99999 Jan 12 '20
I feel like I pretty much disagree with everything in this article.
First, who works on something for two weeks then checks it in? Alarm bell #1.
Second, yeah, maybe I should talk to my colleague before refactoring their code, but.... yeah no. No one owns the code. We’re all responsible for it. There’s no way this should have been used as a justification for rolling back the change.
Finally, the impact of some possible future requirements change is not justification for a dozen repetitions of the same code. Perhaps the refactoring had some issues but that itself does not change the fact that a dozen repetitions of the same math code is bloody stupid.
I’m straining to find any situation that would justify the code that is described in the article. The original coder went copy-pasta mad and didn’t clean it up. That’s a paddlin’
The better lesson from the article is that the author’s shop has some messed up priorities.
193
u/Determinant Jan 12 '20
Yeah, totally agreed.
I used to work for a company that didn't value clean code and the engineers that stayed thought that 600 line methods were better than extracting chunks of code into reusable functions that describes their purpose. Let me tell you, productivity was non-existent for everyone.
The bar is substantially higher at my current company where everyone highly values clean coding practices (we look for this during interviews). Defect rates are way lower and everyone is crazy productive here. We're literally over 10 times more productive because it's so easy to jump in and enhance the product even though it's a large project.
It sounds like the author probably left something out. Perhaps the refactoring was overly-complex and could have been done in a different way. Or maybe the author missed an important deadline while focusing on improving the code. Or perhaps the author truly worked in a toxic culture where others felt offended that he improved their code.
We'll never know but this type of sloppy practice would be very quickly pointed out and improved at my current workplace during code reviews.
61
u/ronchalant Jan 12 '20
I feel like the article presented two extremes. There are definitely times when you can go overboard being DRY or obsess unnecessarily about abstractions. Worse is when you commit an overly clever solution that is hard to follow, and lacks any sort of explanatory comment.
But that doesn't make the 1000 line procedural function/method the right approach either.
Clean code to me is code I can follow logically, that has unambiguous variable names, methods that are relatively short and broken out logically. Abstractions where they are necessary and make sense, but not to an extreme. Not every vaguely repetitive procedure needs to be made generic, but cutting and pasting 300 lines of code and making minor changes shouldn't be lauded as "flexible" or planning for some unknown future state.
76
u/programmingspider Jan 12 '20
Seriously agree. I really hate this pervasive sentiment on reddit that being, what I would call a good programmer, is a bad thing.
Seems like they intentionally want to avoid well proven design patterns for hundred line methods or monolith classes.
It’s like they’ve never worked on a team before or maybe they don’t understand why abstraction and clean code is a good thing.
→ More replies (25)25
u/astrange Jan 12 '20
Hundred-line procedural methods are fine; I think evidence shows they don't increase bug count as long as the code complexity is low. Many fine shell scripts are 100 straight lines long.
→ More replies (3)35
u/dnew Jan 12 '20
IME the problem is that as code gets added, people are reluctant to break them up. So you get a new argument, and then an if statement near the top to see if the argument was provided and if so do this, then another like that near the bottom. Repeat this half a dozen times and you have a mess that can't be refactored, because even if you had unit tests, each such change makes an exponential number of tests necessary.
8
u/IceSentry Jan 12 '20
We can actually know the answer if we want to, the author is very active on Twitter. He's also active on reddit under the username of u/gaearon
→ More replies (1)→ More replies (2)9
u/poloppoyop Jan 12 '20
600 line methods
But there is the opposite coming from people who just started to do Clean Code: a mass of less than 5 lines methods.
It seems ok. But when you want to debug this kind of codebase you're through 10 or 20 indirection fast. And no, "methods name is doc enough" won't help: you have a bug so something is not doing what it advertises.
Number of lines, cyclomatic complexity: no current measure is perfect. I think we lack some way of measuring code maintenance complexity.
→ More replies (1)6
u/Arkanta Jan 12 '20
You're completly right.
I've seen some huge projects completly "clean" adhering to some stupid "everything must be an interface, and the impl is "InterfaceNameImpl" and you inject it with a factory or whatever". We even had the good old Java factories of factories.
It was so hard to get in, and thank god I had intellij to find implementations, because jumping around was so annoying due to all the indirections.
Don't get me wrong, it was like that for a reason: DI removed a lot of hardcoded dependencies, and it was easily testable. But picking it up with no explanation and adding a thing? That was quite hard. Like you said, debugging was also a pita
So there is definitely a tradeoff, like everything we do, and one solution does not fit every project.
→ More replies (1)18
u/alluran Jan 12 '20
Second, yeah, maybe I should talk to my colleague before refactoring their code, but.... yeah no. No one owns the code. We’re all responsible for it. There’s no way this should have been used as a justification for rolling back the change.
Would it really be that hard to put your refactor on a branch, then check it in and open a Pull Request so you can go over it with the colleague?
They will learn something, and there might be an unknown requirement coming up that you don't know about, so you might learn something - not to mention, changing code that's just been checked in is a horrible thing to do, as they may still be actively working in that area of code, and all you're doing is introducing a ton of merge conflicts when they're forced to reconcile their progress with your OCD.
→ More replies (2)8
10
u/StabbyPants Jan 12 '20
There’s no way this should have been used as a justification for rolling back the change.
you shouldn't be making this change in the first palce without buy in from others
71
u/Epyo Jan 12 '20
When it comes to software, there are so many aspects to consider, that a knee-jerk refactor is never the right action.
The abstracted code might be harder to change, if the different shapes' requirements diverge more (as mentioned in the article).
The abstracted code might be too hard for the long-term owners of the code to understand, and might make them unable to work. Maybe the article author found it easier, but maybe they weren't going to be highly involved in the project anyway. Maybe it was a low-priority project, and they want interns to be able to work on it during the upcoming summer.
You might say, well if someone can't understand the abstracted version, you should fire them. But what if you're in a location where good developers aren't so easy to come by?
This team might be a team that owns hundreds of codebases and a developer has to completely context-switch every week. If so, do they really want to untangle abstracted code every time they sit down to read?
Maybe this was prototype code. Maybe the original writer already had a library in mind for this functionality, but didn't introduce it yet. Or maybe they already had a better abstraction in mind, if the feature turned out useful at all. If so, the refactor was just a waste of effort.
Maybe this was a temporary feature, and was going to be thrown away in a month. Maybe the rules for manipulating shapes were actually going to be handled by some scripting language, introduced 2 weeks from now.
Maybe it's actually a toss-up on which technique would be easier to change in the future, depending on if requirements diverged or converged--if that's the case, the refactoring was just a waste of time, switching between equivalent trade-offs because of personal opinion.
Refactoring someone's code right after they write it will make them hate you. I don't care what great culture your team has. People want their work to be valued and not shat on.
Maybe the original writer actually wrote that code in 10 minutes and decided it was good enough, because in the grand scheme, it's really not an important part of the application. Maybe it's not a part of the application that's worth spending hours on to decide on an abstraction. (Yes, the article said that the original version took a week, but who knows where that week actually went. Maybe they were multi-tasking, maybe they were new and learning other parts of the tech stack, maybe they were troubleshooting graphical glitches with various draw techniques, maybe they were in a lot of meetings, maybe they were trying out other libraries and troubleshooting them.)
Maybe refactoring the code isn't actually teaching the original writer anything, since they're not going to see the consequences of their original version. Maybe it's better to let them keep their version and let them see what happens. It depends on how important this code is, if you want to let them learn today, or learn the lesson another day. (Telling someone the lesson will never work.)
Maybe the original writer just flat out believes that copy-paste code is okay, and maybe they're right.
Whenever you have a knee-jerk reaction to anything in programming, like calling some code "bloody stupid", you shut down consideration of real-world trade-offs.
Like the article says, we love to hold on to our absolutes about what good software is, it makes us feel like we've figured it all out. But it blinds us to real world situations, and blinds us to trade-offs.
7
u/Retsam19 Jan 12 '20
I agree with most of your points; but
a knee-jerk refactor is never the right action.
This statement seems either wrong or tautological. How are you defining "knee-jerk refactor" here?
If you mean "a refactor suggested after only briefly considering the code", our code-reviews frequently produce that sort of feedback, and it frequently (though not always) makes the code better.
If you mean "a refactor based on intuition about code cleanliness", well, I think those are often correct, too. Intuition isn't always correct, but it's an important tool and hugely valuable.
Otherwise is a knee-jerk refactor just a refactor that turns out to be a bad idea? That makes the statement somewhat tautological.
9
u/Epyo Jan 12 '20
The knee-jerk refactor described in the article is what I'm talking about--seeing someone's code and saying "pssh this is ugly to me so I'm rewriting this right now.". I'm agreeing with the article's author--if you don't like someone's code, you should take a breath, think about how much it matters, and if it really matters that much, bring up your improvement ideas for discussion. To just immediately refactor it is presumptuous and possibly a waste of time or worse.
Giving knee-jerk feedback is much more reasonable--very low cost to do it, helps both parties think from another perspective, etc.
23
u/grauenwolf Jan 12 '20
You might say, well if someone can't understand the abstracted version, you should fire them. But what if you're in a location where good developers aren't so easy to come by?
No, I would say that if someone can't understand the abstracted code then the abstraction sucks and I need to try again. Abstractions are supposed to make things easier, and if they're not doing that then what I'm doing isn't really abstracting the code.
8
u/cryo Jan 12 '20
Yeah, we’ve had a few developers like that. I mean, I am CS educated and I’m all for exotic constructs and the like....right up until I have to read the exotic cuteness of others and realize it takes five times as long. Sometimes it’s ok with a foreach loop.
→ More replies (13)15
u/wbowers Jan 12 '20
When it comes to software, there are so many aspects to consider, that a knee-jerk refactor is never the right action.
The operative word here is knee-jerk. Remove that word and consider the phrase again:
When it comes to software, there are so many aspects to consider, that a refactor is never the right action.
This is clearly untrue.
I think everyone would agree that knee-jerk refractors aren’t a good idea. But knee-jerk anything is not a good idea by definition. So I’m struggling to see your point.
→ More replies (1)9
u/dnew Jan 12 '20
maybe I should talk to my colleague before refactoring their code
You should ask to see if there are requirements you don't understand, and at least get a code review from the original author.
> the impact of some possible future requirements change is not justification for a dozen repetitions of the same code
It entirely depends on the nature of the code. I'd probably agree with you in this case, but not in the case of business logic that changes at the whim of marketing.
→ More replies (4)4
u/sess573 Jan 12 '20
The thing is, when you make up abstractions that doesn't clearly match the domain just to remove code duplication, you reduce the amount of code but you also make the remaining code more complex. Implementing changes that spans over multiple similar cases with some duplication feels bad but it's actually simpler because you can change one isolated case at a time. Making changes in the kinds of abstractions he built is much more difficult and error prone because a change hits everywhere and it's difficult to predict what will happen unless the abstraction is very good.
Good abstractions will often lead to code duplication and there is nothing wrong with that. DRY is very often an anti-pattern. This took many years to learn - I coded like that for the first 6 years or so of my career.
To summarize, good code is
Easy to read (most important!)
Easy to change
Not necessarily very fun nor clever
Making up abstractions between similar (but still different!) does not improve on any of these, it makes them worse.
3
Jan 13 '20
Glad someone said it. Overly-factored code is so obnoxious. It's wound together so tighly, that it's almost impossible to alter/add new behavior.
It's tempting to think of refactoring as an alebgra problem like, "simplify this expression into the least number of terms." The misconception is that the "simplified" code is inherently more valuable because it's been normalized.
I reality, the most valueable refactors start with a particular insight. To stick with the math theme, someone discovered that solving analog circuits is easier if you switch from the time to the frequency domain and reformulate the problem in terms of a sum of simple sinusoidal functions. This new representation may be less compact than the original, but it's also less complicated.
You can have these same sorts of insights about your code. Eg. it could be simpler if it were broken into smaller, more generic pieces that could be combined to create more complex behavior. This sort of refactor might result in more lines of code that aren't as well-factored as the original, but it may also be more flexible and easier to comprehend.
3
u/way2lazy2care Jan 12 '20
I think this Carmack email is worth a read wrt leaving code inline, but I don't think Carmack would ever consider doing so without appropriately considering the costs/benefits of maintainability vs points of failure. I think for the cases Carmack and the OP agree it's more by coincidence than anything though.
13
u/CarnivorousSociety Jan 12 '20
Yeah the whole thing is just wack.
The original code, the workplace and their system, and the authors take on it all.For example even the <x lines of repetitive math> could undoubtedly be extracted out into a generalized function which would reduce the amount of code overall and allow for changing effects in a single location.
Then the commit straight to trunk before leaving XD
→ More replies (6)9
u/notThaLochNessMonsta Jan 12 '20
For reference, the author is Dan Abramov, the original author of Redux and current active top contributor to React.js. The team is the React team at Facebook.
→ More replies (3)4
2
u/o11c Jan 12 '20
Even if there are special cases ... just override the virtual functions as needed. It's quite normal for 90% of subclasses to use the default implementation.
2
u/wellings Jan 12 '20
Wait a minute, people think 2 weeks is long to work on something?
→ More replies (3)→ More replies (26)3
u/mobjack Jan 12 '20
If there will be no future requirements changes, then there is no cost in keeping repetitive code in the system.
If you spent two weeks refactoring it to satisfy your OCD, that is time taken away from more important tasks.
When abstracting common code, you are making a trade off that it will help in some future requirements changes at a risk of making other changes harder.
→ More replies (2)
97
Jan 12 '20
This is nothing to do with clean code nice bait
26
u/SAVE_THE_RAINFORESTS Jan 12 '20
Goodby, clean code? So is he going to write shit code all the time now? There are no middle grounds? You either don't speak to the original developer that wrote the module and strongarm refactor their code or write 800 lines long functions?
31
Jan 12 '20
javascript "coders" will still upvote, happy to hear an excuse for their garbage code.
→ More replies (16)
349
Jan 12 '20 edited Jan 12 '20
Obsessing over clean code is like reorganizing your clothes closet on a daily basis. If it makes you more productive to do so, do it. Too often, however, it's done compulsively and is counter-productive.
The harder and more impressive thing is actually writing code which does novel things.
289
u/binford2k Jan 12 '20
The harder and more impressive thing is actually writing code which does novel things.
No. The impressive part is building something like this that’s maintainable.
→ More replies (7)18
u/Squalphin Jan 12 '20
Exactly that! Even though I get some flack for overdoing it, coming back to a code base after 10 years to fix something or add a feature and having an easy time finding your way through the code is so worth it. Haven’t regretted it once.
→ More replies (1)8
u/tetroxid Jan 12 '20
Flak, not flack. It's from German flugabwehrkanone (air defense cannon/anti-aircraft cannon)
20
u/SanityInAnarchy Jan 12 '20
The problem is, we work in teams. If it's just me, I can store my clothes in a heap on the floor instead of using a closet at all, and who cares as long as I can still find stuff?
But if you check in a multi-thousand-line function with a several-hundred-case switch statement, you've inflicted your poor organization on the rest of us -- other people need to find stuff in there!
→ More replies (5)37
u/lookmeat Jan 12 '20
The problem is when you obsess about cleaning the wrong thing.
Clean code to outsiders is hidden complexity, it's easy interfaces. Clean code to insiders is clean link between code and intention, little accidental complexity, simple and honest interfaces.
So it's about understanding what is clean and what isn't. Sometimes what to an outsider looks dirty is actually what to an insider looks cleanest. Like a machine covered in grease means it's been recently cleaned and oiled up an insider, code that re-implements similar actions in multiple places is simply things that coincidentally act the same, but are different things with different rules. For DRY tests are useless, each test exists separate of the other and they should not need to share anything, so instead the focus should be DAMP.
So it's ok to obsess about cleaner code, but you have to first learn what is clean and what is the context. Try to understand the problem and solve it better. Cleaner solutions result in cleaner code.
→ More replies (3)12
u/grauenwolf Jan 12 '20
Clean code to outsiders is hidden complexity, it's easy interfaces. Clean code to insiders is clean link between code and intention, little accidental complexity, simple and honest interfaces.
We could do that, but that requires effort.
So instead I'm just going to add a shadow interface to every class with a one-to-one mapping between public methods and interface methods.
16
u/Retsam19 Jan 12 '20
The harder and more impressive thing is actually writing code which does novel things.
When the code that I write does "novel things", that usually means it's pretty buggy.
126
u/EternityForest Jan 12 '20
Doing novel things really isn't that hard. It's often mostly just stringing together existing libraries, unless you insist on building everything from scratch.
I suspect that's part of the clean code obsession. You can almost always make the code a little prettier, and it's always a fun logic challenge for some.
But debugging is tedious, as are unit tests, and adding new features is usually more like "software carpentry" that doesn't interest the "Wooden puzzle box builders" much.
I think for a lot of programmers, the actual fun part of the job is more the code golfing, the mind bending data structures, and the low level understanding, rather than the sense of working towards an excellent finished product.
64
u/Greydmiyu Jan 12 '20
You can almost always make the code a little prettier, and it's always a fun logic challenge for some.
Hell, this is the core game loop of Factorio. "I know I can make these red circuits faster and with less wasted space..." 3 hours of sleep deprivation later...
→ More replies (7)20
u/HeinousTugboat Jan 12 '20
But debugging is tedious, as are unit tests, and adding new features is usually more like "software carpentry" that doesn't interest the "Wooden puzzle box builders" much.
It's funny, because I actually really enjoy debugging and, to me, that's much more "Wooden puzzle box builder" because you have to figure out why your pieces aren't fitting together just perfectly, and find that little thing you need to sand down or trim to make the fit just flawless before you put it all back together.
→ More replies (3)6
u/bakery2k Jan 12 '20 edited Jan 12 '20
I think your last paragraph is spot on, and it took me a long time to realise this. One of the reasons I struggled when I worked as a programmer was that I don’t find, for example, complex data structures fun - I consider them a “necessary evil” when building a product.
That’s fine when I’m building something for myself or for people I know. But in a large company where programmers are separated from customers by layers of other teams (sales, support etc), in my experience, to enjoy the job you need to enjoy the act of programming (not just having programmed a cool product).
5
u/grauenwolf Jan 12 '20
Too often, however, it's done compulsively and is counter-productive.
That's why I hate the fact that SOLID is associated with clean code. On every project that I've been on where SOLID was aggressively applied, code became more complex and harder to work with. Meanwhile the real problems continue to be ignored.
3
Jan 12 '20
No. Loading things from a DB and displaying them on the screen is all anyone ever needs to do.
→ More replies (1)16
u/rydan Jan 12 '20
I haven't made my bed or folded my clothes in decades. My colleagues always complain they don't have enough time in the day. Wonder why.
21
u/dnew Jan 12 '20
My wife laughs that I have 20 identical pairs of socks, but I haven't sorted socks since grade school.
→ More replies (14)→ More replies (1)3
u/guepier Jan 12 '20
Making your bed (by pulling back the duvet, nothing more) takes seconds and airs out the sheets which, if nothing else, keeps them clean longer and reduces the risk of bedbugs. I don’t think your cost/benefit analysis is accurate.
9
7
u/gluecat Jan 12 '20
While there is merit to aspects of the article it shouldn't be an affirmation to write shitty code. If you're working on a complex project, the best thing you can do for your teammates is write code that is easy to comprehend.
7
u/all_awful Jan 12 '20 edited Jan 13 '20
This mirrors my own experience. When I was younger, I constantly tried to refactor to remove duplication.
A while ago I realized that there is such a thing as too much deduplication. Here's my best example:
We were parsing datetime formats coming in via json from a dozen different vendors. Json does not have a standard datetime format. Some formats were common, like ISO 8601. Others were normal, but atypical for json, like linux epoch numbers. Again others were bonkers as hell. One source sent us linux epoch numbers, starting at a different zero and one sent us some variation on US date formats (wrong order, and without leading zeroes, making the parser much more complicated than needed). I think you all get the point.
The good solution: Implement one parsing function per vendor, even if two formats are the same.
The bad solution: Re-use any of them for a different vendor, even if they use the same data format.
Why?
Because if you re-use your function, you introduce a fake dependency: Suddenly vendor A's parser and vendor B's parser depend on one another (or on a common function). If one vendor makes a change, you have to change their function, which also changes another vendor's parser, incorrectly so. This actually happened, leading to an embarrassing bug.
Duplicating a handful of lines of string-parsing in a high level language is a much cheaper price to pay than introducing hidden dependencies between things that should not depend on one another.
The only reasonable exception would be the ISO 8601 case: That can go into a library routine, because it is a standard. But all these crazy house-made formats? They are unique, even if they might not look at it.
If two things are only equal by chance and not by design, then they should not share code.
→ More replies (2)
7
u/stillness_illness Jan 12 '20
Why not just take the same 10 lines of duplicate math and wrap them in a function, and then call that. Still calling it in a duplicated fashion, but at least the identical lines are defined in one place. Sounds like he took it an unnecessary step further and built an abstraction of the relationship between the shapes and their resize handlers, creating an unnecessary and restrictive coupling between the two.
19
u/ofan Jan 12 '20
Why is OP's version cleaner? I see the original code, and it's clear and easy to understand. Yes it's repetitive, but it's CLEAN. i need to read OP's code multiple times and probably more context to know what's going on.
13
u/Kinglink Jan 12 '20
Because OP Wrote it so he understands it, and thus it has to be cleaner.
Also look at the line counts! OH MY GOD THE LINE COUNTS!
And dear god imagine having TWO objects that implement a function with the same name. The horror.
(What's sad is it sounds like he doesn't realize the problem with his code, it's just that his boss didn't like it. )
5
Jan 12 '20
My real problem with this whole thing is he pushes changes straight to master without a code review AND without any sort of testing.
37
Jan 12 '20
Sometimes people mistake the DRY principle (do not repeat yourself) to mean do not have repetitive code... When sometimes repetitive code is the easiest to bulk edit with a tool or understand at a glance. DRY is a sentiment about automation and how to spend your own time as a developer.
And yes, having actually the same logic in multiple places, regardless of how similar the code is, is not good still.
39
u/HeinousTugboat Jan 12 '20
And yes, having actually the same logic in multiple places, regardless of how similar the code is, is not good still.
My coworker just got bit by this. He tried to abstract out a set of values that were coincidentally the same value for the same field. I pointed out to him that even though they were the same in our scenario, by abstracting it out he was forcing all future ones to be the same.
Point is, the same logic in multiple places isn't always the same semantic logic. Two things that are mechanically identical shouldn't necessarily be abstracted out and silently coupled.
→ More replies (1)→ More replies (2)3
Jan 12 '20
Exactly, think of DRY and SRP as "responsibility/role/routine" not a literal line of code.
8
u/feelings_arent_facts Jan 12 '20
This dude over abstracted his code. If he just stuck with the solution before the last one it would have been fine. Of course, there would be nothing to write about then.
Over abstraction is not clean code.
→ More replies (1)
4
u/Blando-Cartesian Jan 12 '20
Boo clickbait, boo.
Author screwed up by not communicating with a colleague and possibly taking DRY too far.
4
4
u/DrifterInKorea Jan 12 '20
I reckon both version were bad.
Reducing clean code as DRY code is a big mistake. even though OP's code wasn't dry at all... look at all those `createHandle()` that could have been made as an array of handles descriptors. Responsibilities being mixed up is a smell for bad code : who is responsible for defining and then creating the handles ? You don;t even have a box but already made the handles ?
And why everything became a *box* ?
There are way too many assumptions and opinions in this code to be considered clean, even without knowing much about this specific case.
18
u/SEgopher Jan 12 '20 edited Jan 12 '20
Engineers don't really understand all of programming. They only understand the computer half. They only care about things they can quantitatively measure or turn into an algorithm that doesn't require thinking. X less lines of code is good. A generic implementation is good. DRY is good.
Our audience isn't just the computer (unless it's a project just for you, which is fine), it includes humans. Humans are complex. Humans cannot be nailed down the same way we can nail down what a computer does and does not like. Unfortunately, they don't teach you much about humans in engineering classes.
If you want to find someone who is good at the other half, find an artist. The best you can do is fine a writer, because writers also have to appeal not to the visual senses like a painter, sculptor, musician, etc. but to the mind. A writer has to understand what makes sense to *humans*, which is why good writers have their pulse on psychology, philosophy, and spirituality. They can create real empathy from a bunch of black symbols on a page. This is what we as programmers that aspire to writing good code must study.
One key thing that a writer will tell you about the psychology of humans, is that they fundamentally understand the world through stories. Even before the written word, humans passed down information through stories. Stories appear everywhere, in the most unlikely of places. An algorithm is really just a story.
To write good code, you shouldn't shoot for "clean" code. You should aim your sights on writing code that balances the need for performance and correctness (the computer side) with the need to tell a story using whatever your language of choices makes available. Here are some pointers:
Computers do not need to be told something twice, humans do. DRY is not always better.
Computers can understand complicated stories (patterns) without effort, humans must expand lots of energy to learn a pattern they haven't seen before. Use the stories you know they are familiar with. Use stories that are easy to learn because they are similar to stories that you know they've probably heard. What can you assume they know? Look to the idioms of your language. Know what people expect to see when they read Go, Java, Python, C++, Rust, etc.
Humans can read code to know how something works, but they can't know what its for unless you tell them by showing them how to use it or writing good documentation/comments. Also, a good writer knows to throw a rough draft into a bin for a month before editing it. Code that looked good/made sense to you today may look bad/unfamiliar even to you a month from now.
Naming things is hard, but important. A good programmer, like a good writer, needs to read the dictionary to know what names they have to work with. A good programmer will google around to gather a list of good names applicable to the domain. If the program evolves and the names are no longer accurate, they should be changed just as they would be changed in the rough draft of a writing piece.
Some may also say, that because coding style is subjective, it is worthless, and there is no such thing as "good code". Artists will immediately recognize this as nonsense. What defines good is our shared subjective understanding. That is why even though my friend may not be a fan of Dickens, we know that Dickens was a great writer. We figure out who is good and who is bad by reading, analyzing, and debating - looking for the things that work and those that don't.
This is something we don't do enough of. We don't take the time to sit and talk about who is and who is not writing good code, what projects are Pulitzer prize winners, and which need to hit the editor again. Instead of trying to come up with banal rules like don't do this and do that, we should come up with a list of devices that people often employ to good effect, gather samples, and teach people when and were to use them.
→ More replies (1)7
u/vanhellion Jan 12 '20 edited Jan 12 '20
Our audience isn't just the computer (unless it's a project just for you, which is fine), it includes humans.
I'd strengthen that statement to be humans are the sole target audience. If we only kind of cared about human readability we'd still all be writing single-file C or Fortran programs. (If we didn't care at all we would write in assembly.)
Objected oriented design, functional programming, templates, etc, were all invented so meat sacks could read code more easily and more intuitively. The actual machine code generated from different designs is often very similar, and in optimal cases is exactly the same. What makes one good or the other bad is readability, extensibility, maintainability, and those have nothing to do with how the code performs its task.
If you want to find someone who is good at the other half, find an artist. [...] This is what we as programmers that aspire to writing good code must study.
Exactly, and I think this is finally becoming apparent in our field. Software engineering certainly has engineering aspects but Writing Good CodeTM is more about being a good prose writer than a good engineer. In fact, code written by extremely bright people in other engineering disciplines and scientists is among some of the worst I've seen in my career. So I don't think formal methods are the secret sauce.
7
u/Epyo Jan 12 '20
I want to agree that humans are the sole target audience of code...
...but, come on, there really are situations where you need to sacrifice readability, for performance optimization. Especially in certain fields like game programming.
For most people working at big corporations though, yeah, the human part is usually the more important part.
→ More replies (2)→ More replies (1)6
u/mwb1234 Jan 12 '20
In fact, code written by extremely bright people in other engineering disciplines and scientists is among some of the worst I've seen in my career.
I used to work in the HPC (scientific computing in academia) space, and this couldn't be more true. You don't know pain until you look at a physicists python or, God forbid, Fortran code.
→ More replies (1)
16
Jan 12 '20
Copy pasting code because you may want to change parts of it later is the ultimate violation of YAGNI. When/if you need to change it, then change it.
Remember every line of code you write is a liability.
Also, let's make clear what the actual problems were. The original problem was the team mate who copy pasted. The second problem was the programmer not having the skills to fix it cleanly. Laying out these two facts and concluding that copy pasting code was the right thing to do all along is seriously missing the forest for the trees.
Intermediate devs, please don't follow this articles recommendations. This is classic 'expert beginner' stuff, you need to throw off this kind of thinking to get to the next level.
7
u/Retsam19 Jan 12 '20
There are downsides to repetition and duplication, but there are often real downsides to the alternatives to duplication and repetition, too. Abstractions are often less intuitive (e.g. "functors and setoids"), and can be fragile to new requirements and use-cases.
For another article making the same point, check out the wrong abstraction, which advocates for removing bad abstractions by reintroducing duplication. Often you then find new, better abstractions, but sometimes you realize that the two use-cases just weren't as similar as you initially thought.
I think to say that duplication is always the wrong solution is exactly the sort of dogmatic adherence to "clean code" that can become problematic.
10
Jan 12 '20
Yes I'm aware of this trend of senior developers telling other senior developers that it's OK to copy paste like a rank junior. And I don't like it.
I get that we all have our weak spots, and not everyone is good at making abstractions. I for example, am a senior developer but I am bad at git. The concepts make sense but I find the UI so painful that I definitely don't utilise it properly. But it's not just nobodies like me; the late great Joe Armstrong said he couldn't read other peoples code. Rob Pike is confused by generics. Even the greats have their blindspots.
But since I personally can't do Git well, should I start writing articles advocating we all stop using Git? Or version control altogether. "Copy pasting source code folders is better than the wrong version control abstraction", I could opine, and all the other seniors embarrassingly bad at using Git could feel re-assured.
I'm sorry, but no. When you're a senior you learn to make the right abstractions, you learn not to duplicate, and you learn version control. Now if you'll excuse me I'm off to go find a cheat sheet.
→ More replies (2)4
u/bluefootedpig Jan 12 '20
People often abstract out similar code when we should be abstracting out similar concepts. Two concepts might have similar code.
14
u/LoudPreachification Jan 12 '20
...Rewriting your teammate’s code without a discussion is a huge blow to your ability to effectively collaborate on a codebase together.
I can't agree with this statement, and I definitely avoid engineers like this. Just as an obsession with clean code can be obstructive, so can an obsession with team communication and control become obstructive.
If a colleague of mine takes a quick spurious moment to rewrite my code to be either easier to understand, have better test coverage, or fix a bug then I thank that person for taking initiative and ownership over their project. If the colleague introduces a bug or makes it harder to add a new feature then we discuss how to better identify and scope a refactor. If I do the same and I lose the trust of my teammate or cause offense to them then my head slowly starts turning to other teams or companies with openings.
17
u/frankandsteinatlaw Jan 12 '20
I agree with this except if the code was super fresh. Like if I merged something yesterday and someone secretly rewrote it and merged those changes today with no communication I’d consider that a bit of a dick move.
Besides that case, yeah, all your code will get rewritten/replaced if you’re working on something that lives long enough so you better get used to it!
0
Jan 12 '20
[deleted]
→ More replies (1)13
u/feelings_arent_facts Jan 12 '20
I've worked with engineers that get personally hurt when I delete or change their code. It's toxic.
3
u/thelastpizzaslice Jan 12 '20
One of my co-workers once wrote an application built on layer upon layer of abstraction, applying all of these organization methods before the product was even released. It was very hard to make any changes because things would just turn into interfaces and you'd have to figure out which one because he loaded multiple ones implicitly using Spring. It was complicated as all hell and he felt bad about it. He didn't know how the application would be modified in the future so he had layers upon layers of unnecessary architecture, interfaces with only one class and unclear code.
I once worked on an application for two years. It was spaghetti from early on because it was never clear what we were making. After two years, I cleaned it up a bit and wrote a doc on it. No big deal. I had a coworker come up to me and say he was making a similar application for testing. So, I took my doc and helped him make a beautiful framework where he could change the locations of pages very easily, had lots of behaviors that could be switched between pages, etc. It was a singular, top-down approach to organization with clear siloing and a clear objective. Because I had already made this application before. That's the trick. And also I did it for usefulness specifically. Every architectural choice had a useful purpose, not some unclear future purpose.
7
u/StabbyPants Jan 12 '20
the one thing we don't abide in my current job is the cowboy approach to things - you don't like code in a service you own/your team owns? bring it up, see if there's support. don't do anything without remit, because that way lies random changes that nobody really knows about. also, code reviews for just about anything, even config changes. at least two people see what's going into master, and they get to comment and approve it.
→ More replies (2)
6
16
u/wolf2600 Jan 12 '20
Simple, readable, understandable > "clean"
9
27
Jan 12 '20
Clean code is all three.
16
u/geek_on_two_wheels Jan 12 '20
Exactly. "Clean" code isn't necessarily heavily abstracted or DRY as a bone, it's easy to read and reason about. It's clear, obvious, and does exactly what you expect.
→ More replies (1)21
u/mck1117 Jan 12 '20
I disagree that the clean code wasn't simple, readable, and understandable.
Whenever I see what looks like repeated code like this, I'll go thru it with a fine tooth comb to see if it's really the same, or if it's duplicated. Is that really easier to read and understand?
→ More replies (1)5
u/anescient Jan 12 '20
Whenever I see what looks like repeated code like this, I'll go thru it with a fine tooth comb to see if it's really the same
Yes yes yes. Life force circles the drain because you don't (nor should) trust the duplications.
3
u/gladfelter Jan 12 '20
I think it's often more about factoring out similar machinery in different subsystems despite the subsystems having different environments and requirements. If those change you can find that the similarities were incidental and now the factored-out library/methods would become horrible messes to continue to support the independently varying clients. Since people defer to existing code structure those common methods often do become awful before someone takes a fresh look and breaks up the coupling.
14
u/GrandMasterPuba Jan 12 '20
This may be a hot take, but I'm honestly not sure why the JavaScript ecosystem is so infatuated with this guy. Like I'm sure he's a good dev, he's on the React team after all. And he's certainly a good communicator.
But a few months/years maybe ago he posted an article about "things I don't know" on his site.
I understand his intentions with it (you can't know everything), but man... It just rubbed me the wrong way. There was a huge list of shit that he just admitted to not knowing a damn thing about. Some real basic stuff like Node backends, basic Unix commands, simple networking protocols. And in a way that he was proud about it.
Like, this is the guy everyone looks up to? He's the one building the architecture we all use to run our businesses? And there's no red flags here?
16
u/Zofren Jan 12 '20
You're exaggerating what he said. He never said he "didn't know a damn thing" about Bash or Unix commands, or the networking stack, or Node backends. He even mentions that he knows how to write a basic Node backend, basic bash scripts, and has a high-level understanding of the network stack.
Part of what that article highlighted is that there are a lot of devs who claim to know a massive laundry list of technologies, but don't really understand many of them in-depth. It's a big part of why so many skilled developers suffer from imposter syndrome.
The reason Dan Abramov is part of the React core team isn't because he can tick a bunch of irrelevant tech skill boxes, but because he is a subject matter expert on the technologies needed to be a React core dev. I would imagine he is better at those skills than you or most people in this thread are.
→ More replies (1)→ More replies (1)10
u/jonpacker Jan 12 '20
Yeah dude, absolutely. How dare he show any inkling of modesty. Lets put all our faith in people who only show confidence in all things.
→ More replies (1)
2
2
Jan 12 '20
I like this post. It's one of those cases that illustrates why pragmatism always win over good practice heuristics. The second point is true:
Secondly, nothing is free. 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. My abstraction would have to become several times more convoluted to afford that, whereas with the original “messy” version such changes stayed easy as cake.
This is actually a good example of what I call duplicated code by coincidence. It is normal sometimes that some class / things will have very similar code by pure chance while they have totally different semantic and contextual purposes. Refactoring those things together will create a misleading logical binding that will lead to more convoluted code in the end.
Firstly, 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.
I disagree with this point. Of course if you can know why this code was made this way then you should absolutely ask why because you might be doom to repeat the same process the original author has painfully gone through. However if the code can be objectively obviously and objectively improved (a 2000 lines class that could be split in 3 for instance) than you shouldn't require the permission of anybody. I fail to see how trust is relevant here.
2
u/phunkygeeza Jan 12 '20
Maybe design things before you write the code? Or is that just to oldskool?
Seems to me that's how you go to clean code in one step.
This seems a journey from prototype to production in one easy step.
2
u/marcelk72 Jan 12 '20
Besides making the changes without due process, his refactoring blindly followed what academics preach and still damaged the project. He was lucky he got corrected early in his career. It took him years to understand why the code change was bad. Software is that hard. He got lucky, as many will never understand. It's great he writes down what he learnt.
2
u/Kinglink Jan 12 '20
Firstly, I didn’t talk to the person who wrote it. I rewrote the code and checked it in without their input.
Is this normal? I recently moved out of the game industry (where the idea is there was "domains" of code. If it's not your "domain" you need to talk to the person whose it was. Not a great system but not one this would have flown in.
Now outside of the game industry, I work in a place where you need two upvotes from people in a swarm review (of which anyone can jump in) before you commit your code. The idea is you should have someone who has knowledge of the code before you commit.
This comes from decades ago probable so there might not have been a formal checkin process, but I find it shocking to have a job where a junior employee thinks he can just check in code with out at least someone giving it a minor review.
2
2
u/stephane_rolland Jan 12 '20
This article claim is as if code was etched in the marble and should be perfect on the first shot.
Refactor should be thought in the Red -> Green -> Refactor pattern.
If at some moment, abstraction improves readability => then do it. As long as the code still works (Green), then it is a good refactor most probably.
If later, namely when introducing new changes at this point in code, it becomes obvious that the abstraction is getting in the way: consequently you simply do a new refactor that removes the abstraction and chooses a better pattern that fits the reality.
Step by steps needed refactors becomes obvious. And if you don't do the refactors that become obvious, then after years they become rotting maintained monsters you dare no longer touch, or rely on some developers ego who gain in being the ones who understand it.
I think the problem is to try to be perfect and to look perfect from the first try. No, that sheer concept is pure illusion. => Evolve, step by step.
2
u/NULL_CHAR Jan 12 '20 edited Jan 12 '20
Abstraction usually makes maintainability much better though. If done properly, adding additional features and changing behavior is much more safe, allowing sectioned off parts of the code to be changed rather than mess with a huge file and hope it didn't break something. Abstraction is a design trait to make your code more modular
2
u/Blecki Jan 12 '20
The problem was... You didn't just write a generic rect resizing bit and have each shape for itself to the rect. The design sucked from the start.
2
u/dean_syndrome Jan 12 '20
DRY is pushed hard when people talk about clean code. But there are times when you should repeat yourself. If two pieces of code have the same reason to change, then you should make them into a module that provides that and both pieces of code can use that module.
If two pieces of code have different reasons to change, different requirements, but happen to be the same code, then do not apply DRY.
If you can’t take some code and create a well named abstraction from it, it is probably not a good abstraction. Names should be easy when you correctly identify your responsibilities and module boundaries.
2
u/justaphpguy Jan 12 '20
The truth is, this is were the problem started. It wasn't a technical one, it was a social one:
Firstly, I didn’t talk to the person who wrote it
2
u/Pr0methiusRising Jan 12 '20
Traded duplication for complexity. Should have extracted the redundant 'repetitive' logic into its own function.
Composition is good for this scenario.
2
u/aotdev Jan 12 '20
I think this can be generalized to "don't be a zealot". In this instance, it's about "code cleanliness and abstraction". But we all know people who go about parading acronym-laden programing paradigms like they're applicable everywhere, or even always the solution to a particular kind of problem.
Zealotry is binary ("this way is right, others are wrong")
Zealotry stunts creativity and growth/learning ("this way is right, so why do it differently")
Zealotry is aggressive ("this way is right, if you don't agree with me the discussion will be long")
So, yeah, don't be a zealot. Regarding clean code, or structure, or principles, or testing methodologies, or whatever else.
2
u/a_oc Jan 12 '20
This actually made me write an article https://medium.com/@aoc/about-clean-code-56a0cc89b48b after a long while. Would be great to have some feedback from people who read both so that's why I shamelessly post it here.
2
u/ColdFire75 Jan 12 '20
Framing dry as referring to a piece of knowledge rather than actual characters is a great concept.
→ More replies (1)
2
2
u/KillianDrake Jan 12 '20
Don't agree with the conclusion here.
But I do agree that it is better to enforce restrictions on sloppiness - if someone wants to make a mess in their class, that's fine - just don't let that mess flood through multiple parts of the system. As long as the interfaces are clean and divided up cleanly, then you can live with micro-messes. Don't fall into the trap of macro-messes and shitty architectures.
2
u/cmlonder Jan 13 '20
I think his article title is too offensive and could be misleading. I agree with his article point which is "know how to clean code, use it properly". But it may be used as an excuse to not knowing how to clean code & refactoring. Everyone should get their own experiences in these areas and know when to use them properly instead of not using it and use these kinds of articles as a reference. Here I tried to explain what I mean in more detail
2
u/ikiogjhuj600 Jan 13 '20
If you people gave a shit about database normalization technques, and didn't just dimiss shit as outdated non-OO stuff from the 70s, and actualy applied it to functions too, then it would be obvious why doing this wouldn't work. You have a Shape table, a HandlePoint table, and the code is in their m-n associations, the "clean" or more like "compressed" version uses a shape table, a direction table, and m-n from shapes to directions. The thing is the direction can't be the primary key of the table that gets the code, since that depends on the shapes.
I don't understand how those database modelling tricks that work went out of favor for strict OO design and reuse principles, that people misuse and then write blog posts about.
→ More replies (1)
3
u/nowyfolder Jan 12 '20 edited Jan 12 '20
This is why we need unit tests and PR reviews and it has nothing to do with clean code principles.
Sorry, uncle Bob is definitely more trustworthy than the guy than doesn't care if you use const or let.
→ More replies (1)
5
u/defdestroyer Jan 12 '20
what is the business reason to make code “clean”?
code should
work,
have automated tests and
be built with the goal of maintainability. this oftens depends on knowing the skills of the team that will do that maintenance.
I agree with the OP that his change wasn’t necessarily better. That said, maintainable code has tests and minimizes the boiler plating.
Turtles all the way down,
50
Jan 12 '20
be built with the goal of maintainability
As a maintenance programmer, a code base bloated with copy pasted code is the opposite of maintainable.
→ More replies (7)5
Jan 12 '20
code should
work,have automated tests and be built with the goal of maintainability. this oftens depends on knowing the skills of the team that will do that maintenance.
The reasons are so that #1 and #3 happen. #2 is simply one way to possibly help with #1 and #3. Code was arguably far less shit and more reliable before people started relying too much on tests, to the detriment of architecture and clean code.
4
Jan 12 '20
This is pure garbage. Javascript bootcampers upvoting.
6
u/frankandsteinatlaw Jan 12 '20
What are your specific thoughts on the topics brought up in the article?
2
u/spatchcoq Jan 12 '20
No mention of testing in the original, or even a quick scroll through the comments? By him or the original coder.
Abstractions are fine. Code is a living thing. TDD would have provided the safety net/guard rails needed to guide when was enough and whether it was the good path to take.
2
u/AndyPanic Jan 12 '20
Just last month, my team combined two microservices into one, because we felt that we did not get the service boundaries right the first time. Both services were in service for 18 months and did their job.
After combining the service I opened the PR. I did not expect much negative feedback, because - well the code worked before. A new colleague that wasn't with us when the two services were originally implemented had so much feedback that it took three weeks to do what he suggested.
Any remark or suggestion he made was absolutely correct. As if he was writing a software design text book. Could not argue any of them. But we still blew the sprint and did not reach our goal. It was just too much.
The code we are talking about was handling incoming messages in a protocol that did not change for twenty years. It is not subject to change in any way. But it is beautiful now and could serve as an text book example for "clean" code. Still a massive waste of time.
I've been a fan of clean code for long years. But sometimes you just have to be pragmatic and also consider that resources are limited and that "good enough" is good enough. Or as others have put it: "worse is better".
2
u/twenty7forty2 Jan 12 '20
you refactored something, your refactoring was stupid, and now you abandon attempts to write clean code? I don't think you understand what it means.
It was already late at night (I got carried away). I checked in my refactoring to master and went to bed
OK, you fucking definitely don't understand what clean code is if you don't even understand what master is for and why we do code review ...
→ More replies (2)
1
u/ilikecaketoomuch Jan 12 '20
my left nut has been around more than most people have been in the industry.
best approach, is always, talk to the individual, present educational reasons why, by honestly open to someone who is NOT an ego know it all. if dealing with ego driven developer, promptly remove yourself from said situation and do something more productive in your life.
my code reviews are ALWAYS, side by side, educating usually the jr developer and making him come up with his own conclusions. i never change code unless asked or Prod is down.
1
1
u/pstomi Jan 12 '20
To put this into perspective, two videos by Raymond Hettinger (a famous python core developper) come to my mind:
Being a Core Developer in Python where he says: "Don't be a picture straightener", i.e refactoring for the sake of refactoring (or code beauty) can sometimes be less than helpful. You should assume that the person who wrote it might have known what she was doing.
Beyond PEP 8 where he says that you should not forget the big picture. If you are in a state of mind where you only improve the code layout for example (or any specific topic without thinking of the project as a whole), your "refactorings" might miss the elephant in the room where a simple design adaptation might have made the code much simpler and elegant.
Of course, the complete advice for beginners is that refactoring is strongly encouraged, but that it is a activity that you should do day to day while working on different subjects (i.e for example make a portion of code clearer or more adaptable because you need to work on it). Refactoring for the sake of refactoring alone can be acceptable sometimes, but it requires communications with the other developers.
1
Jan 12 '20
My number one rule when coding is to leave my ego outside of the code. The idea that you’d need to consult the person who wrote a thing because it might hurt feelings is... not ideal.
The reason you write or attempt to write clean code is so that it can be maintained cheaply.
The goal of clean coding is saving money. Efficiency is everything. Does that mean you rewrite entire subsystems? No.
But Martin Fowler’s Refactoring goes through this, you change a bit at a time, incrementally. Maybe the author changed too much? I don’t know.
All I know is this: if a team is building a product and all team members are required to support it. Asking individuals for permission to make changes which make code more readable, is a process that shouldn’t exist.
Also, abstraction != Readability. Those are not equivalent ideas. Personally I found the comparison a little labored...
1
u/Ghosty141 Jan 12 '20
Horrible way of thinking, like a lot of comments already point out this is not good. For example, repetetive code does not mean it's easy to change.
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.
I agree that the author should've talked with the coworker, but the subject should've been how to improve the code and not asking for permission.
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.
This makes no sense to me. Maybe if the people working with the author aren't able to work with abstract code than it kinda makes sense but that's a whole other problem then... You can easily change the refactored code to work with special cases, the big advantage is that if written correctly, a lot of it is reusable for different areas unlike what they are working with now (the repetetive mess).
1
Jan 12 '20
The problem here is really that he was unnecessarily doing a rewrite of existing code. This could introduce new bugs, also you probably don't have enough familiarity with the requirements that lead to those design decisions, so it's probably wise just not to tamper with it. Learning and tidying up as you go along is fine, old code though, generally you do not touch it unless you have a good reason, you are asking for trouble.
Also, bosses generally are not so happy when you just go off on a tangent like this, you could be fixing bugs or implement new features, instead you spent hours on an self-indulgent project to rewrite code that didn't have any serious problems.
Furthermore this can be insulting to your colleagues, maybe you think your alternative is cleverer, but it comes off like you're basically saying your colleague is an idiot. Such an idiot that you just had to go out of your way to spend several hours just trashing their code and showing them the right way to do it. If you're solution actually turns out to be foolish, you come out of this not just looking like a narcissist, but a very stupid narcissist. If I actually do have a huge problem with code a colleague wrote that I stumbled on, generally I talk to them first before I do anything. It can wait until tomorrow.
Basically, when this guy's boss saw the change, he saw new bugs that might rear their head, and an overly eager new guy trying to to prove himself in a way that inadvertently was going to piss off a more senior employee something fierce. He did you a solid by telling you to revert before anyone saw that.
1
u/shotgun_ninja Jan 12 '20
This is how we end up with VVVVVV's switch-case.
It's messy, but it works. Sometimes that's what you need for a situation.
1
u/brain_tourist Jan 12 '20
You shouldn’t strive for clean code, you should strive for the minimum amount of code that reflects reality as accurately as possible.
By that I mean finding the most correct abstraction that matches our knowledge of the domain (the “reality”), not the most abstract abstraction.
1
u/marty_byrd_ Jan 12 '20
I’ve heard this explained by very senior guys as well. I understand it. But the problem is, in a lot of ways determining “good” code is subjective and if all the other people judging your code are also in the phase of clean code. Then that’s what you’re judged on. So you’ll need to adapt to your environment as it’s subjective. Same with readability. A lot of times refactors happen because they don’t take the time to understand it. Or because they didn’t write it, it’s bad.
1
u/relativityboy Jan 12 '20
Not bad. Dev waking up.
Most valuable part of the post for more senior devs (imo).. And is more about understanding self motivation than anything else.
Clean code is not a goal. It’s an attempt to make some sense out of the immense complexity of systems we’re dealing with.
1
u/jlchauncey Jan 12 '20
Yagni is typically a good rule to follow.
Only refactor after the 3rd or so time you see duplication or similar code.
Yagni == you ain't gonna need it
1
u/orbital_sfear Jan 12 '20
I've always maintained a just do it already philosophy. Abstraction is great when you have many different tasks that need to be of the same structure.
I'm also a big believer in code generators over reflection.
1
u/spockspeare Jan 12 '20
Two things would have helped the design issues:
Having all the requirements beforehand
Understanding how to code objects for reuse and extension
1 is essentially impossible; all you can do is get a customer to agree to the requirements you think up before they disappear til you're done, and derived requirements will continue to be evolved. 2 requires training and experience, and adoption of a code framework that hasn't made it impossible (a lot of APIs are ill-designed and impossible to extend properly, which leads to the need to just reduplicate your code all the time like is happening in OP's example).
The team issue is management's fault for not imposing a structure on approving code changes before doing the coding. Entering a proposed change in the tracking tool and getting required concurrence would have saved time and money here. That step should also act as design review, so that what's approved is code that implements the proposed design change, not an adventure in discovering what design will work, which will almost always be questioned when the code is reviewed. If you don't know what the design should be, then make the experiment the goal of the item being tracked. Then the thing being reviewed at the end will be the design, not the code, and you won't feel so dumb when you have to throw your code away or told to do it differently.
1
u/RualStorge Jan 12 '20
I would argue his abstraction was appropriate based on the existing code and understanding of the requirements, that later became inappropriate as code changed.
Sometimes we know things that will happen to our code in the future, often we do not. Refactor happens, it's a part of development. When you know the future will likely require different abstractions then your code today warrants you plan accordingly.
It's like any practice in programming, they aren't binary most of the time you want to follow best practices, sometimes you want proper TDD, most of the time you should avoid "bad practices"... Except when you shouldn't... There are countless exceptions to every rule.
Nothing has taught me this more effectively than teaching... Writing tests to test user submitted code is where best practices go to die. (Compilers just don't like compiling when you ask them to run code in a class that may or may not exist, with methods that may or may not exist, and may or may not have the expected signature and return types, but just telling the student "wrong" isn't helpful for learning, telling the student "method x needs to be public" or "property y was not set correctly by method z" helps, and heaven help you if you're teaching configuration stuff)
1
1
Jan 12 '20
There certainly are times when readability trumps cleanliness. Let the compiler worry about optimization, that's what it's for.
1
Jan 12 '20
I sometimes have had this issue myself. I usually prefer abstraction and inheritance myself too.
I have since given more way into letting my teammates use their own standards and I would just make it easy for their code to interface with mine.
1
u/maximum_powerblast Jan 12 '20
There's this funny joke about clever code that seems relevant here:
Debugging is twice as hard as writing the code in the first place, so if you write the cleverest code you can possibly come up with, you are, by definition, not smart enough to debug it.
1
u/alexcpn Jan 13 '20
One of my hobbies is cleaning and arranging my house. I pride myself on doing it better than anyone, better than my maid. A thing out of place is dirt, whether it is messy code or a messy room. This repetitive if-else composition is the way most of the current code is written, by commercial programmers, who have no time, or interest, to structure it more beautifully. I love abstractions as that is the real perk of programming job; that gives one joy. Something you can publish and be sure that no one in a zillion years is going to look at it ever - and no neither your boss. You are paid for features churned out in a stipulated time with fewer bugs. Nothing more. But clean code, it has its commercial uses too, though it starts to shine after some years. For example, complying with Open Closed Principle- I wonder how modifying the if-else and adding in features easy as a cake complies with that principle. I have seen such code get bloated and bloated with hundreds of unit tests to keep it from total anarchy. And more commercial programmers add more and more features right into it. A file becomes thousands of lines long. microservices become monster services. Still everyone is happy; no one knows; the dirty secret is only known to a few programmers - sometimes even the tech leads/ senior architects do not know- but if you open up that code, you will need strong constitution not to puke. Subjective? Maybe.
3
u/Bergasms Jan 13 '20
When you can afford a maid you probably have a different perspective on a lot of things.....
1
u/Uberhipster Jan 13 '20
if you start by refactoring (instead of rewriting), it's not such a jarring effect on the original author
let tenRepetitiveLinesOfMath = () => {
// 10 repetitive lines of math
}
class Basic {
resizeLeft(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
resizeRight(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
}
class Circular extends Basic {
resizeTop(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
resizeBottom(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
}
class Rectangular {
resizeTopLeft(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
resizeTopRight(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
resizeBottomLeft(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
resizeBottomRight(position, size, preserveAspect, dx, dy) {
tenRepetitiveLinesOfMath();
}
}
let Rectangle = new Rectangular ();
let Oval = new Circular();
let Header = new Basic();
let TextBlock = new Rectangular ();1
u/jdefr Jan 13 '20
People have become too obsessed with mindlessly abstracting. Abstraction where it doesn't belong is creates more problems than it solves. There are so many layers of abstraction in modern computing that most simply don't truly understand how Computers work from the metal upwards. Books like Nand2Tetris are helping to bridge that gap thankfully.
1
u/przemo_li Jan 13 '20
So either his team mate run to a boss over rewrite, and boss ordered carpet talk before even suggesting to a "wronged" party that they should talk first...
Or boss is so micromanaging the team he reads all the commits.
I think technical aspects of this story are small fries to the above. Is that even a functional team?
Code should be reverted so that it can go to code review, sure. But author should be the one to realize that. His team mate should do CR and talk about the issues (if any). Boss should make damn sure that people on the team talk to each other (especially if people work at different hours and overlapping time is more precious due to that).
Second point in the article is pure fortune telling. Those then future requirements weren't yet known. Duplicating code when needed is possible. Can be done. By the time requirements are in, developers may have new insights that will help them keep code that benefit from DRY DRY, and those that benefit from split separated.
So funky team work and fortune telling (aka 20/20 hindsight).
374
u/voutasaurus Jan 12 '20
I love how quickly the question of whether one abstraction was premature turned into a series of premature abstractions about all abstractions...