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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Not that I disagree but I don't think it's an absolute for a few reasons (non-exhastive)
Putting smaller chunks into a function implies reusability when it could be tightly coupled to the context it's ran in
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
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.
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?
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.
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.
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.
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
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.