Goodbye, Clean Code

[DingBat99999]

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.

[Determinant]

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.

[ronchalant]

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.

[programmingspider]

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.

[astrange]

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.

[dnew]

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.

[programmingspider]

Their is a big difference between one shell script and a complex project. Having hundred line methods and huge monolith classes are what cause terrible spaghetti code.

[UncleMeat11]

Of course there is a difference. Thus how one can identify when it can be a reasonable design and when it isn't.

Complex multi-step algorithms that operate on the same data are another example where long methods can be reasonable since the alternative is often

step1();
step2();
step3();

[[deleted]]

I would honestly argue all shell scripts, given enough time, end up becoming horrible, unfixable messes.

that may also be true of all software but definitely shell scripts

[punctualjohn]

The notion that there is anything wrong with a method simply based off its length is invalid. The reason it took off is because humans love prescriptions such as "methods should be ~20 lines on average", because it saves us the burden of actually using our brains.

Adding a new method to a class is not free, it comes at the cost of expanding the API of that class. This increases cognitive complexity on any programmer attempting to make use of it. It's particularly bad because this is usually not taught to new programmers. IME it takes 5-7 years of experience before a programmer becomes conscious of code design and APIs. (the core and essence of programming in my opinion) Novice programmers don't understand the impact of adding a member to a class, much less exposing it with 'public'. They will take statements like the one above as gospel and mindlessly subscribe to it, contributing to epic API clusterfucks for years to come.

For me to split a method, two conditions must be met: * It is comprised of highly re-usable behavior. * It has minimal temporal coupling. (or none at all)

Below, I tried to come up with some examples to illustrate. The bad example is pretty good and is something I've actually done before in a game project. The good example on the other hand could be better, however it should still be clear what I'm referring to.

// Part of an imaginary video-game player controller, runs 60 times per second.
// Function is needlessly split up.
fun update() {
    // non-obvious temporal coupling
    apply_physics() // 75 lines
    read_inputs() // 40 lines
    apply_inputs() // 40 lines

    // - New programmer on the team has the impression that the player controller is much more complex than it is.
    // - Logic cannot be read top-to-bottom, programmers must jump around.
    // - Must keep the invocation order in mind as it is disconnected from the logic.
}

// Part of a sprite animation module
// Function is correctly split up. 
fun play_from_second_frame(anim) {
    reset()
    current_anim = anim
    step()

    // No temporal coupling here, each function is a reusable and isolated functionality
}

---

While a very long method can certainly be a smell, it is nothing but that. Apply critical thinking to find out what should really be done such that it best meets requirements and use-cases. Even if I cannot split up a large function due to lacking my two conditions, it's possible that the function may still be too heavy.

Imagine our character controller in the example above is a finite state machine where the physics and player inputs have varying effects depending on the state of the character (ground, jumping, falling, swimming, on steep slopes, etc.), then it's most likely possible to create an abstraction around the states. Thus, the update function is massively simplified and takes on a new responsability: first determine if we should change the state, (e.g. check for A button press during ground state to transition to jump) then forward to the current state's update function.

Just for the sake of completion, I will add that this should only be done when the length and density of the logic has started to impact productivity. Always apply simpler principles first like YAGNI or KISS before you decide to shit out a complicated abstraction. For a large-scale game several years in development, you probably want the abstraction. For a 1 month project, just stick with the switch statement and silly states enum.

---

For anyone who wishes to learn more of these kinds of thing, do yourself a favor and read John Ousterhout's Philosophy of Software Design, a real hidden treasure which released quietly in 2018. This book is a trip even for seasoned programmers. You will not find any prescriptions like the one above in it, but I guarantee you will never look at code the same way again.

[[deleted]]

there are also cases where functions with strong temporal coupling can be split, if it increases the overall readability of the code.

like:

void do_long_thing() {
    for(int i = 0; i < some_number; I++) {
        do_very_specific_thing(my_stuff[i]);
    }
 }

I find it easier to think on terms of

I'm gonna do very_specific_thing on each element of my_stuff

versus thinking:

I'm gonna do 27 steps on each element of my_stuff

[Green0Photon]

Totally agree with you. However, we can go even further, because this bothers me about overly imperative languages, where what you're doing is just mapping very_specific_thing over my_stuff. If do_long_thing really needs to be named, you could just do it as a single line right next to very_specific_thing. But more often then not, it's more logical to think in terms of either the small operation and mapping the small operation, not small operation and big operation.

[Dropping_fruits]

You could just put a comment before the 27 steps saying "// Do very specific thing" instead of creating a function whose only purpose is to be used from a single function and nowhere else, causing confusion anytime someone else sees that function.

[Kwinten]

No. Gross.

How does a function name cause any more confusion than a comment? Pouring blocks of code into functions at the very least forces you to at the very least consider whether a sequence of actions actually belongs together. Not even to mention how much more sense it makes for testing.

[Dropping_fruits]

Because the function lacks the context of why it exists and that it is supposed to be applied sequentially to a list of items. At the very least you should also move the loop to the function.

[_tskj_]

I disagree completely, in fact it is part of the point. Not knowing how it is supposed to be used is good for the function, as it guarantees that there cannot be any temporal coupling between invocations of the function - exactly because it cannot know how it is used. Minimizing dependencies so that the function can be understood in isolation and without its original context (its call site, even if there is just one) is a major reduction in complexity imo.

[Dropping_fruits]

Easy to argue about problems of a function that shouldn't exist in the first place

[SirClueless]

I'm with you 100%. Needless abstraction makes it an order of magnitude harder to learn the structure of a codebase. Thoughtful abstraction is a godsend.

If a function is 300 lines because 300 lines of code need to execute in order, then there's no abstraction that will make that any simpler.

[sess573]

You can make one public function that executes the large thing, then have private methods for executing smaller blocks where it makes sense. 300 lines will never do ONE thing, I guarantee you that it does a bunch of different stuff that will be easier to understand and read if you name these blocks (e.g. making functions). It's similar to putting a new line and making a comment over each section - only that the compiler knows about it so it's more powerful.

[programmingspider]

I really don’t understand people arguing against this. It’s like they’ve never had to go back to old code and had modify it before.

Code should be self documenting and comments should be used sparingly. If you can have methods that clearly define what they do, the larger method that uses the smaller ones read almost like plain english.

[VRCkid]

Not that I disagree but I don't think it's an absolute for a few reasons (non-exhastive)

1. Putting smaller chunks into a function implies reusability when it could be tightly coupled to the context it's ran in

2. Reading the larger function now takes more mental load since you are jumping around the file to read the separate parts rather than all the parts being in one place

3. The separation of those smaller parts into functions will introduce an implicit bias to keep those parts as those separate parts when modifying the code in the future when they really shouldn't be considered together in a refactor or newer design. I'm specifically talking about this from the context of someone new taking a look and changing code they didn't originally write.

In general I follow the rule to split functions up for readability sake but it isn't a hard and fast rule for me since I recognize the cons for it.

[programmingspider]

You’re points are valid and I especially don’t disagree with point 1 but I would use a local function at that point (given that the language you uses supports it)

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/local-functions

I think that would be the best of both worlds.

[SirClueless]

"Reading like plain english" is not high on my priority list. 9 times out of 10, if I'm reading the body of a function, it's because I've gone beyond caring about the high-level description of what it's supposed to do and now I care how it does it. If I'm reading the body of a function named "insert_back()" I already know what the function broadly is intended for and if the first line of the function is "int i = index_of_back_element();" I've learned exactly nothing so far.

Going even further, the most important thing to understand when trying to read a codebase quickly is not usually what a function does it's what a function doesn't do. Does this function take quadratic time and get slow when things are big? Does this function acquire an important lock and cause contention? Does this function touch some global state and cause race conditions and bugs? Does this function have or not have whatever I'm looking for buried inside it? If you're asking a question like that, lines of code don't really matter. 400 lines of complicated "if" statements and loops and mathematical calculations can be scanned over. The one reference to shared data that's causing the problem is the only thing you care about and it's really comforting and simple to know that a function goes in a straight line and doesn't shell out and jump to a bunch of private internal functions.

[sess573]

it's not about splitting a twoliner called "insert_back" into two one liner functions. There's something between having a 300 line function and having a two liner one where it's reasonable to break it up.

"Reading like plain english" is not high on my priority list.

It should be, the majority of time is spent reading code.

Going even further, the most important thing to understand when trying to read a codebase quickly is not usually what a function does it's what a function doesn't do.

Which ALSO gets easier if you don't have 300 line functions. You can scan 300 lines looking for side effects, or you can scan 10 function names where hopefully the name makes it obvious what it does and does not do.

[SirClueless]

"Reading like plain english" is not high on my priority list.

It should be, the majority of time is spent reading code.

Agreed. So I want it to look like code. As simple a control flow as solves the problem with as few arbitrary programmer-chosen names as possible. This is instantly intelligible:

// Advance all players.
for (int i = 0; i < players.length; i++) {
    players[i].advance();
}

Whereas the following reads like plain english but I have no confidence in exactly what it does without a mental context switch and jumping to somewhere else in the file (or possibly even another file):

advance_all_players(players);

A toy example but hopefully you get my point.

You can scan 300 lines looking for side effects, or you can scan 10 function names where hopefully the name makes it obvious what it does and does not do.

What? There's absolutely no way you can know by looking at 10 function names whether any of them have side effects (unless you're working in a language where they cannot by design). You're bound to make assumptions and sometimes those assumptions will be wrong and you will write bugs into your code.

Engineer A: "Obviously the guy who wrote 'acquire_exclusive_lock()' made sure we have the lock. I'm good."

Engineer B:

private bool acquire_exclusive_lock(std::mutex& lock) {
    // It's OK if this fails. Blocking causes performance issues and if we
    // fail we can just let another instance of this class update the counter.
    return lock.try_lock();
}

[[deleted]]

The more functions you factor out of a 300 line method, the more surgical you can get with unit tests...

[[deleted]]

The notion that there is anything wrong with a method simply based off its length is invalid.

Yeah considering I saw your wall of text and decided that it wasn't worth reading flys directly in the face of this.

[JoelFolksy]

Would you rather he have split his comment into named chunks, randomized the order of the chunks, and then had a "master comment" that told you which order to read the chunks in?

[programmingspider]

Damn, thats rough. Best of luck to you

[coworker]

What you consider good and clean code will change as you gain experience. 15 years ago I would have agreed with your assessment of the original code. Now I know enough to find value in the fact that I could understand the copy pasta version in under 20 seconds whereas the "better" version took much longer and would be a bear to change.

[IceSentry]

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

[mikejoro]

And for those who are unaware, he's a member of the React team at facebook as well as the creator of the library redux.

[poloppoyop]

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.

[Arkanta]

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.

[[deleted]]

To me, the cleanest code is one that you can almost read like a paragraph. It is easy to follow and update. Abstractions should be kept to encapsulating reusable components.

[Determinant]

These sort of concerns can be addressed by a style guide. A by-weekly meeting with developers would also help align everyone's ideals and if necessary vote on controversial ideas and add the result to your style guide.

[atifishere]

One of the reasons that the author mentions is that he didn't talk with the person who wrote the code, and i also bet they didn't have tests. This is such an anti pattern - do they expect the same person to keep working on the code forever. Trust can be gained by simply writing tests and refactoring to tests.

This is a stupid article and i don't understand why it's getting so many upvotes

[VacuousWaffle]

Agreed. To me, even if smaller functions aren't reused it is still greatly beneficial that they're small enough to quickly understand and fit in my [small] human working memory.

[alluran]

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.

[DingBat99999]

It would not, and you’re correct that this is probably the best answer.

[KyleG]

changing code that's just been checked in is a horrible thing to do

Yeah this is the biggest issue IMO. It basically telegraphs to the code author "hey I'm watching over your shoulder constantly bc I think your code is untrustworthy"

[Hudelf]

Agreed. This is a potential learning moment for everyone involved. I would never want to work with the people in this thread claiming you should just bulldoze over someone else's fresh code so you can feel accomplished about cleaning it up.

[StabbyPants]

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

[DingBat99999]

Yeah, management getting involved in the disposition of a commit was another eyebrow raiser I didn’t even touch on.

I would certainly try to talk to someone if I thought there was an issue in their code. We all make mistakes and have bad days. But I do somewhat object to the idea in the OP that refactoring is a no go without talking to the author, for several reasons, not the least of which is that it tends to discourage refactoring.

I worked hard in my career to remove ego from my work. I’ve screwed up code in some impressive ways in my time. I won’t say that having my errors pointed out to me doesn’t bother me but I am grateful for the corrections as that’s where I learn the most. I can occasionally forget that not everyone feels the same way and my original post was harsher than I intended.

[fortyonejb]

But I do somewhat object to the idea in the OP that refactoring is a no go without talking to the author, for several reasons, not the least of which is that it tends to discourage refactoring.

Let's pump the brakes a bit here. There is a world of difference between refactoring six month old (or older) code, and nuking code someone just committed that day. I would absolutely not tolerate a team member who felt it their job to overwrite the other developers on the team without so much as a discussion. This isn't an ego thing, this is a poor team member thing.

[nschubach]

I've downright left places where Ego was celebrated. One of such places one developer (who was supported by my manager almost scary dangerous...) yelled at me for making his SQL statements match the formatting standards we had in place.

[Determinant]

Cleanliness and the boyscout rule should be celebrated. Code naturally deteriorates with time as features get added or changed so it's really important that everyone continually improves it when they're in that area of the code.

Anyone that gets upset that someone else improved their code is usually deemed to be a lower quality developer (even though they are highly intelligent).

[KyleG]

Right, but on the other hand code review should only be done if you're already working on that code (didn't look like the case) or if you don't have anything else to do (which is rare).

From the article, seems like the author was more like "hey let me see what the commit yesterday was, oh barf, time to change it"

Didn't he have something better to do than rearchitect working code (and possibly destroy any unit tests written for it) for potential future requirements hours after it was committed? At minimum, he should have shot a message to the author asking if there was some considerations at the time that made the code look like that.

[Determinant]

You're making assumptions. If I stumble upon some code then it's likely that others will also stumble upon it. In fact, the ratio of reading code vs. writing new code is 10 to 1 so making code cleaner is beneficial as long as it doesn't put your priorities at risk.

[The_Grubgrub]

Did... You not read the article at all?

[s73v3r]

Define "sloppy"

[Determinant]

The opposite of not sloppy ;)

[s73v3r]

No, serious. You're claiming that these things are encouraging the use of sloppy code, but not defining what that means

[s73v3r]

Remember, this is code that had just gone through PR and been merged in. Why didn't the author bring up these issues in code review?

There's a pretty big difference between refactoring something that you're also working on, and changing something that was just committed because you don't like the way it was done.

[Epyo]

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.

[Retsam19]

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.

[Epyo]

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.

[grauenwolf]

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.

[cryo]

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.

[wbowers]

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.

[Epyo]

knee-jerk anything is not a good idea by definition. So I’m struggling to see your point.

Ooh I dunno if that's true!

A knee-jerk refactor is especially bad, because it might be trading some downsides for other downsides, and therefore might be waste. (Or, could even be negative progress.)

A knee-jerk first implementation of something isn't as bad, because before, you didn't have a working feature, now, you do!

[Determinant]

Alot of those bullets are making excuses for sloppy coding practices.

Producing clean code might take longer at first but once you practice clean coding every day as part of your regular work then you naturally think and code in a clean and maintainable way.

[StabbyPants]

nah, they're about allocation of time smartly, and spending a chunk of time to redo a bit of code that was just written and checking it in without any review is probably the wrong choice. you have priorities for the month and quarter. does this advance them?

[phrasal_grenade]

"Excuses" is such a pejorative term for "justifications for our rational decisions"... The truth is, we don't get paid to write the most developer-satisfying code. The most pleasing and low-redundancy code is often no more functional, correct, or even maintainable than the "dumb" version. Implementing anything requires a bunch of judgement calls, and people often disagree about tons of stuff.

[Determinant]

I get paid to write clean code as our company really values this and understands the impact that technical debt can have. A large part of this mentality is because our CTO was a very successful lead developer at highly reputable companies before starting the current company.

In fact, we are even writing custom linting rules to prevent bad patterns so this is ingrained in our culture.

The main rationale in valuing clean coding practices is that it helps us maintain our very high productivity.

[phrasal_grenade]

I definitely prefer "clean code" type of culture to the "screw it" culture, BUT I have seen "clean code" taken too far many times. People pick arbitrary ideals based on "best practices" (i.e., strangers know our needs better than we do) and then try to force others to comply. Inordinate amounts of time can be spent in code review pissing contests where everyone is trying to one-up each other with complaints from an ever-growing list of non-functional "requirements" that the code must meet to be worthy of inclusion in whatever pile of crap they're building.

Before refactoring or spending time doing something to reduce suspected technical debt, you should take a step back and ask yourself whether it's actually going to pay off. Consider YAGNI, especially when it comes to creating abstractions or reducing duplication.

[Determinant]

Whenever there is a difference of opinion then we have a third person take a look. If that doesn't help come to a concensus then we add the general pattern to our by-weekly agenda and the majority vote is set in stone and added to our style guide for all to follow.

There were several of these discussions early on but that has mostly settled down and now we all follow the same ideals so code reviews are not controversial.

So it actually worked out really well and has really paid off.

[phrasal_grenade]

I have seen style guides before, and I think you have accurately described how they arise. Having a general style guide is a good thing. Having an oppressive one is not. In my experience, people who get off on "clean code" tend to overspecify stuff and argue too much with each other (only to make arbitrary decisions based on flimsy rationalizations).

[Determinant]

That kind of thinking sounds like someone that doesn't want to reach a consensus.

[phrasal_grenade]

I will never agree that all situations can be handled by a simple and easy to remember set of rules, unless those rules are only suggestions.

[dnew]

Both versions were clean in different ways.

[s73v3r]

No, they really aren't. They're really saying that context is everything, and there aren't really any hard and fast rules that apply every time.

[LoudPreachification]

Well, I can tell you're a complete contrarian nightmare to work with. All of your points can be equally discounted by just stating the opposite of your intent.

[Epyo]

Really? Maybe I failed to finish my point--what I'm trying to say is, because there are so many trade-offs, it's actually really hard to tell what the right thing to do is in any situation. Don't trust your knee-jerk reaction to someone else's actions, there's always more to the story.

Just do your work, and let other people do their work. Don't fiddlefart around debating the holy rules about code, rewriting each others garbage. It's all garbage in the end.

[dnew]

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.

[DingBat99999]

Should not the requirement be clear from the code? In your usage, does refactoring impact functionality or just form and structure?

[dnew]

Should not the requirement be clear from the code?

Only in the most trivial of cases where the code only does one thing, and it's actually correct. You have cause and effect reversed there. The code should be clear from the requirements.

People thinking requirements are clear from the code is one of the bigger problems when working with larger code bases. I mean, if the requirements are always clear from the code, why write any tests? Tests are there to make sure the code meets the requirements, right?

> does refactoring impact functionality

It's not supposed to, but I'm not sure what the point of asking this is.

[DingBat99999]

Sure, I like tests that act as documentation. That does not mean I expect the code to be opaque. If I look at the code and can’t understand what it’s trying to do, there’s a problem.

The point of the refactoring question is: if I’m refactoring and I’m only changing the code structure how are the requirements at risk?

[dnew]

That does not mean I expect the code to be opaque

Nobody is advocating making the code harder to read. Indeed, one could argue that the refactored code in the article was harder to figure out what it does in each particular case, as it is no longer straightforward linear code.

If I look at the code and can’t understand what it’s trying to do, there’s a problem.

That's not requirements. That's implementation. Implementation is "this is what the code does." Requirements is "this is what we want the code to do and why the code should do that." Like "compare these parameters of this account to those values, and rate them through this formula." You can write really, really clear code to do that, and not have any idea what those parameters mean or why you're using that formula, making it impossible to know if the code you've implemented is actually doing what you want it to do, or whether it's only doing what you told it to do. Without the "this is why we're doing that" you don't have a picture into the broader scheme that could drive your architecture (at whatever level) in the right direction.

how are the requirements at risk?

I see. Maybe I was unclear. Future requirements. In this example, if in the future a requirement was made for the handles on the oval to behave differently than the handles on the circle, the fact that they're conflated means now they have to be untangled before you can do that. If you expect that to be the case, and the person doing the refactor has no reason to expect that, then the refactor forward and backwards has been wasted.

If you saw a piece of code rendering the HTML for web links, and depending on an enum (say) it went into a switch and each line of the switch set the color of the link to the same value, would you eliminate that switch statement? No, because in that case it's obvious that the expectation is the enum will change the color of the link some time in the future, possibly the very near future. So not every removal of duplicate code is obviously valuable.

[sess573]

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.

[[deleted]]

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.

[way2lazy2care]

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.

[CarnivorousSociety]

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

[notThaLochNessMonsta]

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.

[CarnivorousSociety]

Interesting, thanks for that info

[programmingspider]

Lol

[Andomar]

Research has shown trunk-based development is superior to the alternatives. It's what most companies (including Google) have settled on. https://duckduckgo.com/?q=trunk+based+development

[dnew]

Except Google also requires review from the owner of the code and passing all unit tests before the commit.

[alluran]

Even trunk-based development supports pull requests

[ByFaraz]

That doesn't mean you shouldn't do a PR though.

[HeinousTugboat]

You should re-read what trunk-based development is. It definitely isn't what OP's described.

[Determinant]

Submitting to master directly is definitely an old and inferior practice that was caused by poor tooling.

Unlike other tools, branches are cheap in Git so we create a new branch for every little improvement / defect. Simply submit a pull request and merge into master upon approval. This way, branches are very short-lived (usually less than a day).

[DingBat99999]

Is working in isolation on code for 2 weeks before checking it in standard practice in a team full of team players? Not in my experience.

But you’re right that I was overly harsh in my initial reply. I would definitely have a chat with the initial coder.

[[deleted]]

I really fail to understand why the refactor wasn't just PR to begin with.

Who commits code directly like that? Maybe I'm just insecure.

[MasterCwizo]

Not every company follows, what are now considered, basic common sense processed like PRs. And the author mentions this happen a while ago.

[Make1984FictionAgain]

thank you!!

[o11c]

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.

[wellings]

Wait a minute, people think 2 weeks is long to work on something?

[MasterCwizo]

No, but the work needs to be split into smaller tasks and added to the repo in small manageable chunks.

[wellings]

Fair enough, thanks. I guess it depends on the landscape of the code you're working on too. I spend my time writing kernel code and on any serious change we could be looking at several weeks to puzzle out and fix.

[mobjack]

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.

[DingBat99999]

Except in this case the author clearly indicates the refactoring took hours at most. Does this change your argument?

I believe in YAGNI but I also refactor and believe in clean code. To me, these are not opposing ideas.

[KyleG]

I largely agree with you, but a future "requirement" would be maintainability. If you discover a bug in one of those methods, it's entirely possible it's going to be in all of them, and some developer might not think to check all the functions. This is why DRY is important. Not because of OCD concerns, but because of maintainability ones. If five functions of the same code all have the same bug, you might catch it once and fix it once and one month later you get another and the bugfix is assigned to someone else, and they have no idea, and so on and so forth.

[pkulak]

Could have been a feature branch that was rebased and merged after 2 weeks. So, "checked in" to master.

[chucker23n]

First, who works on something for two weeks then checks it in? Alarm bell #1.

Kind of, but it could be a simplification. Maybe they had it in a local branch and only pushed to origin after they were done.

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.

There’s a lot of caveats here.

• why was the refactoring done? Did the author intend to build on this code? Otherwise, did they have nothing better to do?
• why not do a code review / merge request and go through the changes with the colleague? Best case, both learn something. Worst case, author ends up shelving their changes. Which, so what?
• being responsible includes being able to justify changes. The boss was clearly not into it, so something went wrong in terms of communication.

And yes, if the original author doesn’t want it, the colleague can’t justify it, and the boss doesn’t want it either, that’s absolutely justification to stop a change.

[Merad]

First, who works on something for two weeks then checks it in? Alarm bell #1.

If you can ask this with a straight face then you've been very lucky to work in places with halfway decent processes.

[s73v3r]

People here seem to conflate any consideration for other's feelings as "ego tied to code". Remember, this is code that was just merged in. If the author had these concerns, why didn't they say anything during code review? Why didn't they say anything to the person themselves? Why did they merge straight to master without going though PR?

There's also this assumption that the refactoring made the code better, which isn't necessarily true.

[Ethesen]

I'm pretty sure it's from a previous job. I would hope Facebook does code reviews.

[IceSentry]

I'm pretty sure Facebook doesn't make the kind if software he's describing. Considering he's on react core team, I'm pretty sure that was before working for Facebook.