There's a decent summary here - https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988c...
I found that a lot of those guidelines lead to the exact opposite. Examples:
- Prefer polymorphism to if/else or switch/case (oh, the joy of tracing a simple task through 50 files)
- Use dependency injection (same as above)
- Hide internal structure (that "private" is the default in C++, Java and Rust just adds to boilerplate; the default case is that you want everything public (unless you like writing trivial getters and setters just for the fun of it); legitimate uses of "private" exist, but are rare)
- One assert per test (I guess I need to throw my property-based tests out the window)
Was meant to make a failed test instantly communicate what's wrong with the unit under test. As frameworks evolve and our practices around them change, it's absolutely fine to come up with new rules.
All of uncle Bobs rules come with pages of explanations of what problems they solve. If you don't have those problems, you may not need those solutions. The book is more about the spirit of the law than the letter.
I've read thousands of tests with multiple asserts that were perfectly plain about what was being tested and in many cases the logical sequence made it easier to understand.
Breaking them into separate tests with one assert would simply have meant a shit ton more code to read.
Uncle Bob's "rules" often come with mile long caveats that he seems to be blissfully unaware of. I think I get why he said it - overloaded tests are a thing - but he fucked up trying to turn his observation into a rule.
> Was meant to make a failed test instantly communicate what's wrong with the unit under test. As frameworks evolve and our practices around them change, it's absolutely fine to come up with new rules.
To put a finer point on that then, you're saying the rule is obsolete if your test framework allows assert statements to accept message strings?
Any time a class has invariants to maintain, you probably need private. That could be infrequent in your experience, but overall, invariants are a cornerstone of abstractions.
This is such a strange POV to me. There are cases where your classes are just data records, but any object with logic surely wants to restrict access to its state so it can maintain invariants? E.g. the simplest classes I can think of, like a vector that consists of a pointer to a buffer, a length and a capacity, wouldn't want to provide a setter for the buffer, or allow callers to set it to a random value?
It makes almost no sense to have all private class fields and methods. The only thing I can think of is fat handles with friend classes or functions.
Maybe it's something you could miss if you're coming from a strong C background and still thinking of C++ as C-with-classes, but otherwise it's something any serious user of the language would pick up.
But there are reasons and rationales to "lockdown" the code (beyond not trusting fellow devs!) and at that point I suggest that any mutable state language cannot be properly locked down - so use a functional language where its easier to reason about.
Usually (IMO) private classes etc are just trying to make it easier to reason about state changes - when its probably best to dump the whole mess.
I read that as "one operation per test". Specifically, both:
assertEqual(complex(1, 2) + complex(3, 5),
complex(4, 7));
auto c = complex(1, 2) + complex(3, 5);
assertEqual(c.re(), 4);
assertEqual(c.im(), 7);
assertEqual(complex(1, 2) + complex(3, 5),
complex(4, 7));
assertEqual(complex(1, 2) + complex(-3, -5),
complex(-2, -2));
assertEqual(c.re(), 4);
assertEqual(c.im(), 7);
In cases like this I find myself extracting a helper assert method if it gets used 3 or more times.
e.g.
assertEqualComplex(c, 4, 7);
assertEqual(c.re(), x);
assertEqual(c.im(), y);
This replaces a clear, well defined thing (assert) with a vague, ill defined thing. What constitutes an "operation"?
There's almost no point to the rule if it isnt trivial to distinguish one operation from a group.
"One assert per test" -> just test one "concept" per test, not literarily 1 assert statement per test. IE :
assert(loc.x == 10.3)
assert(loc.y == 10.4)
assert(loc.z == 10.5)
In any case, "concept" being such a loosely defined thing would render the rule a bit pointless.
There is something deeper behind this rule but think he fucked up articulating what it was.
However when you get to larger programs big if ... else blocks become painful. Not having a DI mechanism becomes painful. Encapsulation becomes important because you will find other parts of the program modifying something when it really shouldn't be.
I used a moan about these things until they were taken away and then I really missed them.
Even if you use many files and only put a tiny amount of logic in each of them, you still need to have an understanding of the overall system and how things work together, and scattering things in more places, each with multiple wrappings, just makes that even harder.
I think you forgot one of the most devastating points in Clean Code, namely short functions and DRY (when overdoing it). It leads to the same problem "jumping through 50 files".
My favorite subversive action to write clean code is using jumps. There is no end to how convoluted code people write to avoid the oh so harmful leap.
I feel like much of the need for short functions comes from nesting ifs moving to code more and more to the right. That is easily solved by jumps or early returns. Early returns can lead to too many functions if not doing with modesty, though.
EDIT: Dogmas taking the developer out of his comfort zone is probably the main culprit. Any advice is good, as long as you don't follow it.
Java has the record type. C# has struct and also property members that serve the same purpose.
I bet C++ and Rust also have better options for data records than plain classes.
If this is arduous it may be worthwhile to change your dev environment
In its absence other people take dependencies on internal details which makes code much harder to refactor later. Lack of scoping on visibility increases the surface area of your APIs and increases the likelihood of bugs. Lack of scoping of visibility on state means losing almost all control over preservation of invariants.
Fred Brooks:
> Show me your flowcharts and conceal your tables, and I shall continue to be mystified. Show me your tables, and I won’t usually need your flowcharts; they’ll be obvious.
In that situation, having to sprinkle "public"/"pub" all over them is very noisy and unproductive. It's the worst with Rust's enums, which require you to declare the visibility of variants and enclosed types of variants separately, so you see a couple "pub"-s per line of an enum definition.
That may be an argument for separate keywords for plain data, and for "APIs", although I'm not sure how I feel about it. Underneath it's just coupling of data with functions. Some languages have separate keywords for those, some don't.
I'd say that's only true for objects whose purpose is to merely group semi-related data together for copy/transfer/storage, nothing more. If your object has invariants/rules, it's a completely different story.
>legitimate uses of "private" exist, but are rare
So if you have a domain object with certain invariants (and any serious project has plenty of those), who makes sure its invariants are always upheld? If everyone is allowed to put whatever they want inside your object, it's easy to end up with inconsistent/corrupt data. Ideally, you want all the logic which deals with object mutations and their validation to be concentrated in one single place, and force all clients to use it, to avoid copypasting same code every time you mutate an object (which is error-prone). That single place can be the object itself, in the constructor or the setters, with fields made private (or const) to defend against misuse. Of course, you can validate object state in a separate function and treat your objects as just dumb bags of data, but then we aren't talking about OOP anymore (where the argument "private vs. public" actually makes sense).
I do not deny the usefulness of ADTs (which is what "private" is all about). I question whether it's the right default. My experience tells me it isn't. Whether you call my style of programming "OOP", is irrelevant to me. This term, after decades of evolution, doesn't mean anything anymore. I certainly don't treat it as a badge of quality and don't consult any definition of it before judging whether a particular technique in programming is useful or not.
I remember going over unclebob github a while back and this wasn't the impression I got. The parts I read seemed like obtuse code that added pointless abstraction for the sake of following some convention.
It's that small changes like adding getters and setters give an easy-to-follow PR, a false sense of accomplishment for both the author and the reviewer, and ton of dopamine with that. While important changes, that actually deliver business value or repay technical debt, become increasingly difficult to do without replacing massive amounts of code and redoing the file structure.
Inertia is the biggest demotivator.
In Java (or Java like languages [C#]), which is the language they were really written for. There are many languages for which the precepts of Clean Code are not that great a fit.
Also - It's probably time to stop recommending Clean Code - https://qntm.org/clean
I do think this article makes a lot of valid criticisms and it’s well worth a read. I remember being influenced by it early on but then re-read it as I gained more experience. This article sums up what I felt on rereading.
I hate commented out code, but this doesn't work in every situation. While working on a clinical information system, we had frequent "please bring it back" requests that came a few months after things were removed (also by request). Relying on source control is nice, but unless you document every feature/removal or use very extensive commit messages, finding the code that was removed (especially if it was spread over several classes) is easier if you just comment it out and write why it was commented out.
The important part here is to add why something was commented out.
And commented out code gets stale so quickly. Much better to check out an old version where it still worked than uncommenting a bunch of lines that now have compile errors and have become incomprehensible. Happens so easily, e.g. after some variable renames.
You can guess, but that will leave you with many classes/functions commented out only in the hope that one day one of them will be requested to be put back. This leads to nobody daring to remove old code for months/years.
Then, one day, a new coder on the team just ask WTF is this here, realize it's outdated and not running anymore, and will remove the whole thing. Then, the "senior" guy on the team comes shouting at him/her: "We might need it one day, what do you know about our team, bla bla bla, we had a feature 4 years ago that was removed and we had to add back, so now we have the rule of commenting things out". Yuck, what a nightmare.
Just create good commit messages, don't group irrelevant changes together, and you'll be able to find the commit if you really have to. And even if you can't, you know you can still just develop that thing again, right?
This sounds like you're travelling towards burnout at supersonic speed.
Rapidly changing environment/features is just newspeak for customer has no idea what they even want in the first place and project management is non-existent.
But I do wonder, if the better option would not have been to make the behavior that you commented out optional. Sometimes one can know beforehand whether a request is actually what people are going to want in the end. Once I wrote variant 1 of a certain feature. This was deemed not good and variant 2 was written. This was also not what people wanted so variant 3 was written. By then I got the inkling that also variant 3 would not be there to stay so I made it an option whether 2 or 3 was in effect. In the end, the final functionality was mostly a lot like variant 1 so the option turned out not be useful but maybe sometimes such optional behavior can sometimes be the best way....
git log -p -- fileNot exactly relevant to this discussion, but it sounds like a good case for a config flag. Let them turn it on and off as often as they like.
...for trivial cases. For more complex, real-world scenarios, these kind of rules need to be applied sparingly, and where needed. Otherwise you get things like too much abstraction and several layers of inheritance, making it difficult to figure out what anything actually does - the opposite of nice to read
Uncle Bob et al have some good guidance, but cargo-culting them led to long-lasting, negative effects on some communities, such as Java and C# developers.
<< I would change "one assert per test" to "one concept per test". Uncle Bob makes a fairly big deal about how the former is a misconception. >>
I believe that's because the term Clean can be intuitively defined in opposition of quick and dirty : "I have spent a lot time on it. So it isn't quick and dirty. It's clean."
I have worked with people who were super booked up on Go4 and all the design stuff, and yet they can be fairly insufferable to work with when they parrot design advice from all this stuff, precisely because they can't explain why in every given situation. Then, on the receiving end of things, most of the criticism people who are expressing in this forum wrt to uncle bob, do so about examples where they coming from the perspective of something they understand.
I think there is A LOT of value in all of this experience, but it has 0 utility if you don't understand it. In other words, you are much better off not considering how you feel about code at all, and always sticking to doing things because you understand why. (And I'd say 'understand' means in a way that you can explain to someone else who may be skeptical). Context is key... so it's like if you're about to do a live coding demo in front of the board, then yeah, it's ok to comment stuff out - I think that's obvious. Ideally, your outcomes will be much, much better if they're coming from those 'obvious' points of view in any given situation.
Any just to be fair, doing something because you're curious, or it's fun, is just a good a reason as any, it just depends on the context.
This is why, there are other people who I've worked with, who know all that stuff - I'm talking super senior people - and yet rarely ever cite it or even mention it. Usually, they will ask questions because they're patiently trying to get you (or whomever) to see for yourself the same, all-encompassing 'obvious' reason, that overrides all the other important factors in a given context, that they already see. If you ask them why a piece of code that doesn't 'feel right' to you is the way it is, they'll usually have a story about some situation, where they just had to do it that way, and maybe it's bad now but if you were there, and knew what they knew, you would have probably done the same.
So, my point is, yes uncle bob stuff is fairly sound, but no I don't agree that we should follow it if we don't understand it and we're better off almost always sticking to what we do understand. O but also cool analogy about the quick and dirty. I just realized though that if you invert it completely it's like 'slow and clean'.
Yes, if you take them as general guidelines, not dogmatic rules with variable interpretations
The latter is exactly what his fans do
> 2 .Keep it simple stupid
> 2. Prefer polymorphism to if/else or switch/case.
No, not at odds with one another...
Whatever approach is clean or not can vary wildly by language. Or framework.
I do like clean code, but I think the article has a point: it's a very vague concept.
Cyclomatic complexity would put if/else behind polymorphism while using lines of code and depth of inheritance would favour the if/else way.
That's the crux with software - there's not always an objectively better solution in terms readability/comprehensibility and some metrics that try to capture these qualities are inherently at odds with each other.
- you have the same if-then-else in 3 places in code with 3 different effects
- you refactor it to polimorphism (Enum with a few overloaded methods works well in java)
- now one of these places need additional condition so you make it a little more complex
- year passes
- you have eldritch abomination of OOP instead of 3 if-then-elses with slightly different conditions because at no point someone was brave enough to refactor back into if-then-elses as the special cases piled up
And notice how if one needs to add a new action, all objects are required to implement it. In typical OOP languages that touches many files even if the action is used only once.
So polymorphism works nicely if there are only few actions but many different objects, like in a typical GUI. But in cases of few objects and many actions, for example, doing a lot of different queries over few data sets it leads to a lot of boilerplate. Switch over actions will lead to less code.
It has benefits, if you do not apply it dogmatically everywhere
Suppose the program has to make a choice. When the choice exists only in 1 place, use an if/switch. When the choice exists in multiple places, polymorphism centralizes it and quickly becomes the simple option.
For example, a change in law required us to make a choice based on a date:
If (thing.someBusinessDate < 2010-06-01) { doTheOldWay();} else{doTheNewWay();}
interface TheWay{ edgecase1(); edgecase2();...}
class TheOldWay implements TheWay{...}
class TheNewWay implements TheWay{...}
class TheNewNewWay implements TheWay{...}
TheWay wayDetector(thing){/* all the ifs here */}
That said, I think "simple" if/else or switches are much more common than the complex cases that fit polymorphism, so it seems a bit off to have it as a general rule to prefer polymorphism.
vs
> Needless Repetition.
> Prevent over-configurability,
because you're accepting an infinite range of possible callback methods instead of asking a simple yes/no question.
I don't get why this rule even exists, actually. If you take three boolean arguments, that's not okay, but if you wrap them all into a configuration object and pass that instead, that's suddenly... okay?
Only if you actually call it exponentially many different ways, which seems unlikely.
> I don't get why this rule even exists, actually. If you take three boolean arguments, that's not okay, but if you wrap them all into a configuration object and pass that instead, that's suddenly... okay?
Three or four booleans are not something semantically meaningful that you can reason about. Five or six classes representing the cases that actually occur in your program, that's something you can understand. Often passing a callback function is too.
Boolean flags fall into a hole in the middle somehow, probably because of the exponentially many possible combinations; they look like a finite set of possibilities, but in practice you almost certainly haven't tested all the cases and a lot of them probably don't make sense.
There is no secret to this, it's merely tests, no matter how clean my code is I would not feel confident unless I have tested my changes, whether manually or automatically and manually quickly becomes intractable.
There is the option of formal verification, but I have found the tooling to be disagreeable with me.