My opinion about this constantly changes through my programming experience. Functions longer than 20 lines of code? Pffff, an algorithm/problem can be such complex in nutshell and decomposing a function solving it will be non-relevant (see any std library of PL). Mutable global state? Pfff, anything in software is based on that, the problem is using it properly in target architecture (ex. in micro-controllers global state usually manages analog/digital output, calling a function for this adds memory and time overhead especially in real-time systems). Copy-paste? Pfff, similar code != this code solves the same problem, sometimes it's easier to include a change in "copy-paste" code than deciding how to include this in boundaries of "reusable" parts (ex. for the first time there're two structures X = (A, B) and Y = (A, B) with similar logic but different domains, and then manager decides to include specific logic for Y working with B data).
Now my general principle which distinguishes "good" code from "bad" one, is how good it resembles a model solving given problem. It can be a non-neccessary complexity of code (ex. instead of "if (isFinite(a)) { ... }" writing "if (a.toString() != "Inifinity" && a.toString() != "NaN") { ... }" when "a" is simply a number). And contrary not-enough complexity of code, relying on blind faith (famous XSS vulnerabilities, SQL injections in web apps are examples of that).
Code that relies alot on pointless abstractions (aka. bad "signal to noise ratio").
Code with lots of microdependencies to perform really trivial tasks.
Code with premature "optimizations" (usually done before actual performance measurements).
Code sacrificing readability (and thus maintainability) to be overly clever or to do trivial optimizations (usually with little to no impact on overall performance).
And of course all the different ways to mess up comments, style and versioning (Yes, I consider the versioning part of the code):
"trivial" measure highly relies on programmer's/developer's experience and knowledge. For first developer traversing a tree and applying some updates based on nodes conditions would be "trivial" so thee places all the code in one function. For second developer the same problem would not be such trivial, so thee creates two functions "fn listFromNodes<T, U>(t: Tree<T>, pred: (fn(e: T) -> bool), map: (fn(e: T) -> U)) -> List<U>" and "fn applyUpdates(updates List<Update>)".
> Uncommented code
Why comment a code that's mapped to problem trivially observably (ex. if it's code describing some GUI component with style fields and your task is change style of its part)? I prefer commenting code that contains complex logic (which is measured through code reviews, developers skills, calls etc.) and its effects aren't observed obviously.
> Commented-Out code (git exists for a reason)
Yes and no. Commented-out code is usually in-place saved draft of old code. If some bug is encountered with new code, you can comment-out new code and uncomment-out old code quickly to discover a bug in new code later. Git is less easy to accomplish this task.
> Inconsistent naming
Some rudiment not quickly refactored, may be involved. Example, I had a project when I had type "CardRef" which initially contained only a name (string) of card which was used as a reference for it. Then I decided to include some scores for a card to selecting cards smarter. I kept a name for type the same because it was used a lot in a project for related entities.
"trivially observable" is not a defined measurement however. What a piece of my code does may be perfectly obvious to me, but mystifying to someone else, or even to myself in 4 months. I am not advocating for half a page of comments before each top level definition, or trivial comments like `// this requests data from server` , but a single line of comments above each function or typedef isn't too much to ask.
> If some bug is encountered with new code, you can comment-out new code and uncomment-out old code quickly
I can also check out the commit where it was last changed, stash the info, and apply it to my dev branch. Same functionality, and less cruft in the code.
Let's say the architecture of a house is beautiful, and its a marvel of structural engineering, but; If the floorplan is unintuitive, the stairwells alternate between 3 different parts of the building each floor, none of the rooms are labeled, all the access shafts are of different size, the original architect didn't even keep a copy of a blueprint, and some rooms are simply blocked off with a sign on the bricks saying "maybe need that later"...
...then the guy who has to rewire the cables in the damn thing later is gonna have a bad day, and the guy who has to pay him for all the extra hours is not going to be happy either.
I tend to agree, but with some uneasiness.
I don’t often review the previous commit diffs when working, if I don’t know it is there why would I find it in git?
If each class does one thing, anyone will eventually figure out how it’s doing it. If it’s doing multiple things, all bets are off. It’s also the exact type of code that will break something unrelated when changed.
Mind you, we don’t need classes like DictionaryConstructor and StringParameterParser, but you should - in about one sentence - be able to say what each class’ purview is.
We have a class whose purpose is programmatically setting strings to be displayed. But displaying the string is done two levels further. And the strings are always static. So there's two layers of complexity and a ton of autogenerated code.
So a class with only one function may be appropriate, but it's not the logical conclusion of SRP itself. For instance, in a project you may want to draw something to the screen. It is not the concept of SRP that you should have one module per function, and have a module which draws circles and another which draws lines (that isn't to say this couldn't be a logical and sensible result, but it probably isn't). Instead you have a module (likely with submodules) which is responsible for drawing to the screen, period. You pass it a description of a shape (however that may be done: data objects, objects which can "draw" themselves in some abstract sense, dedicated procedures/methods for each shape that take shape parameters) and it draws to the screen. This is in contrast to spreading the screen drawing routines across a many thousands or millions of lines codebase.
The other thing SRP discourages is, say, that screen drawing module also being your input module. They could appear to be in the same module from a high enough level, but ideally only because a high level module has submodules that contain the separate responsibilities. For instance, ncurses does both output and input, but hopefully (I've never looked at the internals) it's divisible into two sections (each section composed of some number of modules, probably) that clearly are responsible for either input or output, but never (or rarely, and only where it is necessary due to some underlying technical reason) both.
i also see a lot of code where people effectively throw 30 responsibilities into the same class. Really difficult to update. strictly enforcing single responsibility solves a lot of problems.
As a great (but terrible) example, a project at my first job was a real-time system and the dev for it (I was in testing back then) had decided that since he only had 100 ms slots per task, but had the ability to use up to 10 tasks (or whatever the limit was), he'd distribute some logic across all 10 tasks, rather than making one task responsible for that logic entirely. So the system could have had (and ended up having) a reasonable dataflow, DAG structure relating the tasks (input task, fan-out to processing tasks, fan-in to output task), but instead had a much more complicated internal structure than necessary. It was incomprehensible because of its incoherence. Some of the processing tasks were 2-deep or had another layer of fan-out/fan-in, ultimately. But they should have been done that way to begin with. One of the worst bits of this is that it forced an ordering on the execution of the processing tasks that was not, strictly, necessary except for this distributed logic. Imagine a flow more like this:
Versus this: (With some of those tasks being composed of multiple tasks, but in a more coherent way now.)Bad code to me is code that is difficult to modify or extend. “Difficult” to me implies a higher likely hood of regressions or time spent working.
Probably the worst thing of all is the “wrong abstraction”. Concepts that have been shoehorned into other abstractions - bloating method signatures and variables - obfuscating the intent of the names. This issue becomes cancer that causes a ton of other problems.
Great code reads closer to English language. The bottom of the call stack has high level abstract language and the top of the call stack is highly specific. With module on the stack maintaining a consistent degree of abstract language.
Didn’t believe in this cargo from the beginning. Instead I’m watching the world change. When you work with code and understand what it does and all of the implications, these rules are irrelevant, because you have your own sense of when it’s time to (…).
To me bad code is basically two things: (1) negligent/inconsistent formatting, (2) when you read it you can’t grasp what it does. Not because of complex algorithms, but because it is an abomination consisting of tens of files, 5 meaningful lines each, and logic mostly smeared across imports.
My latest favorite was an abstract factory of a single server constructed from specific implementations of property accessors which combine into a hierarchical non-documented method-structure. All it does is listening on a tcp port, regex-parsing messages like
replying “command OK” and calling back .on(“command”, (data_structure_class) => {}). Too bad it parses it wrongly when msg has “;” in it, which is okay according to the spec. 17 files, countless boilerplate lines only to fail in a trivial task, which is basically a one-pager.