7 ms·

> If you follow them you will end up with code that's really nice to read and easier to maintain, and, most importantly, that you can confidently change. I fou

> If you follow them you will end up with code that's really nice to read and easier to maintain, and, most importantly, that you can confidently change. 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)

rootlocus 5y ago
> One assert per test 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.
- pydry 5y ago
  This makes no sense whatsoever. 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.
  - deleted 5y ago
    [deleted]
  - deleted 5y ago
    [deleted]
- MaxBarraclough 5y ago
  > > One assert per test > 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?
MauranKilom 5y ago
> 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 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.
karatinversion 5y ago
> 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) 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?
- tjajab 5y ago
  I guess that for a lot of people it is the case where you have a class with mutable state that is the anomaly.
- yakubin 5y ago
  IME I write about 10-20 as many data records as "classes". I do use "private" from time to time to protect invariants. But if the default was switched to public, my code on balance would be a lot shorter and readable. I'm not saying that "private" shouldn't exist. I'm saying it's a poor default.
  - tubs 5y ago
    In c++ you choose the default so I'm not sure it should be lumped in with the others.
    - rightbyte 5y ago
      In the literal sense private is default though. I wonder why. Lexical sorting of private, protected and public? 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.
      - tubs 5y ago
rhdunn 5y ago
> One assert per test I read that as "one operation per test". Specifically, both: assertEqual(complex(1, 2) + complex(3, 5), complex(4, 7)); and: auto c = complex(1, 2) + complex(3, 5); assertEqual(c.re(), 4); assertEqual(c.im(), 7); would both satisfy that requirement, but: assertEqual(complex(1, 2) + complex(3, 5), complex(4, 7)); assertEqual(complex(1, 2) + complex(-3, -5), complex(-2, -2)); would not. -- In that case, Clean Code is advocating having something like a "test adding positive complex numbers" and "test adding a positive complex number to a negative one". This makes it clear in the test failure report which of the operations and conditions has failed.
- foofye 5y ago
  And then you get a release mode warning about maybe unused c, unless you stick c in your code.
- SideburnsOfDoom 5y ago
  assertEqual(c.re(), 4); assertEqual(c.im(), 7); This is "logically" one assert, on two lines. So I would welcome it 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); which contains assertEqual(c.re(), x); assertEqual(c.im(), y); Which makes that a bit clearer. This is not the best example but you can get the idea. I might have 5-10 lines in an assertTransactionSuceeded(txn) method.
- pydry 5y ago
  >I read that as "one operation per test". 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.
- ziml77 5y ago
  Your interpretation is a saner way to write tests, but I saw no indication that's what he meant. If it was what he meant, then he should have made it clearer. It's not like he couldn't have added an extra couple pages with an explanation and examples.
Toine 5y ago
These are minor inconveniences relative to the massive benefits all these techniques bring to your codebase. "Go to implementation" shortcut is a thing and solves the first 2. "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) is absolutely fine (even you could do it in 1 assert).
- pydry 5y ago
  If he meant one "concept" he probably would have said that. 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.
YEwSdObPQT 5y ago
In simple cases many of the suggestions in the clean code book make things more complicated. 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.
jseban 5y ago
I agree, these rules make the code extremely verbose and over abstracted and scattered. The resulting code base will be several times the size of a straight forward solution, and this has a very big negative impact on maintainability. 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.
rightbyte 5y ago
I agree with my whole heart. 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.
- BrS96bVxXBLzf5B 5y ago
  Could you provide an example of how you would restructure some nested ifs to jumps? I can kind of picture it but can also imagine doing it badly and being in pain.
- ziml77 5y ago
  These are the points that I've come to despise from Clean Code. After I read that book I wrote code that tried to follow all those rules. It didn't seem bad at the time, but returning to code written that way has always sucked hard. You have to jump around far too much with the 7 line function rule. And over-DRY code can actually be harder to change since repetition is often incidental. In that case a change in one place will actually affect more than its supposed to.
BigJ1211 5y ago
Also if you look at the code samples he provides they often don't follow his own rules. (At least that was the case when I read the book, don't know if it's been revised since)
addicted 5y ago
If you’re just using your classes as data records, most languages have better options for you. 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.
- yakubin 5y ago
  Unfortunately, Rust doesn't. Rust only has "struct"-s, no classes, and they're private by default. Enums require the "pub" keyword once before a variant name, and once inside tuple associated with the variant. So often it's two "pub" keywords per line in an enum definition. So after writing code for a while, I then, upon compilation, need to fix up tens of "pub" occurrences. Pretty annoying. Wasted time, and the end result is hideous.
cerved 5y ago
> Prefer polymorphism to if/else or switch/case (oh, the joy of tracing a simple task through 50 files) If this is arduous it may be worthwhile to change your dev environment
- ziml77 5y ago
  I can hit F12 or Ctrl-Click to navigate to definitions and I get a back button to return to where I was across those navigation operations. Yet it still sucks to bounce through all those files. And it's usually not just a matter of drilling down through the calls, but also moving back up to find out how a dependency of a class that I've ended up in is initialized.
barrkel 5y ago
Private - or visibility scoping generally - is absolutely essential when you program in an environment with multiple teams. 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.
- yakubin 5y ago
  I agree. What I'm saying is that private is a poor default.
  - thfuran 5y ago
    Why? I think it's pretty reasonable to only expand APIs deliberately.
    - yakubin 5y ago
      Because most things in healthy programs aren't APIs, but just plain data: records, sum types and composites thereof (arrays, stacks, queues, whatever). 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.
ravenstine 5y ago
How else is "Uncle" Bob Martin supposed to sell more books and presentations if people actually write code they can understand and are happy with? :D
nfw2 5y ago
IMO it's generally good advice to be wary of rolling too many disjoint use cases into one abstraction, and I think that's the rationale behind recommending polymorphism, but the ergonomics of actually writing OO code make it impractical advice to try to follow as a rule. Inversion-of-control is often a more practical way to solve the same problem, in my experience
kgeist 5y ago
>the default case is that you want everything public 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).
- yakubin 5y ago
  I've partially responded to that already[1][2]. Let me add to that, that when you group together data, a lot of invariants are preserved by the logic you already wrote for the types you compose your types of. Most basic grouping of objects is a struct. It's a product type. Often what I want really is the product of all the states of the component types, without eliminating any combinations. And in the case of sum types: I practically always want all the states. All the invariants are already handled in the types being summed. Not for transfer/storage, but because it logically makes sense. Another thing is that "make invalid state unrepresentable" is a good guideline, but it can lead you into a corner, where you write a lot of code just to make sure that your type system proves that a certain error is not representable by a type, when this error is trivial, unlikely and would take seconds to debug at runtime (maybe would even be so obvious, as to not need any debugging at all). Just because a certain invariant is true in a domain, doesn't mean that it needs to be encoded in the type system. We need to choose what's the most productive use of our time as developers. Those are trade-offs. 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. [1]: <https://news.ycombinator.com/item?id=30112738 https://news.ycombinator.com/item?id=30112738> [2]: <https://news.ycombinator.com/item?id=30113932 https://news.ycombinator.com/item?id=30113932>

rootlocus 5y ago

> One assert per test 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.

pydry 5y ago
This makes no sense whatsoever. 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.
- deleted 5y ago
  [deleted]
- deleted 5y ago
  [deleted]
MaxBarraclough 5y ago
> > One assert per test > 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?

MauranKilom 5y ago

> 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 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.

karatinversion 5y ago

> 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) 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?

tjajab 5y ago
I guess that for a lot of people it is the case where you have a class with mutable state that is the anomaly.
yakubin 5y ago
IME I write about 10-20 as many data records as "classes". I do use "private" from time to time to protect invariants. But if the default was switched to public, my code on balance would be a lot shorter and readable. I'm not saying that "private" shouldn't exist. I'm saying it's a poor default.
- tubs 5y ago
  In c++ you choose the default so I'm not sure it should be lumped in with the others.
  - rightbyte 5y ago
    In the literal sense private is default though. I wonder why. Lexical sorting of private, protected and public? 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.
    - tubs 5y ago

rhdunn 5y ago

> One assert per test I read that as "one operation per test". Specifically, both: assertEqual(complex(1, 2) + complex(3, 5), complex(4, 7)); and: auto c = complex(1, 2) + complex(3, 5); assertEqual(c.re(), 4); assertEqual(c.im(), 7); would both satisfy that requirement, but: assertEqual(complex(1, 2) + complex(3, 5), complex(4, 7)); assertEqual(complex(1, 2) + complex(-3, -5), complex(-2, -2)); would not. -- In that case, Clean Code is advocating having something like a "test adding positive complex numbers" and "test adding a positive complex number to a negative one". This makes it clear in the test failure report which of the operations and conditions has failed.

foofye 5y ago
And then you get a release mode warning about maybe unused c, unless you stick c in your code.
SideburnsOfDoom 5y ago
assertEqual(c.re(), 4); assertEqual(c.im(), 7); This is "logically" one assert, on two lines. So I would welcome it 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); which contains assertEqual(c.re(), x); assertEqual(c.im(), y); Which makes that a bit clearer. This is not the best example but you can get the idea. I might have 5-10 lines in an assertTransactionSuceeded(txn) method.
pydry 5y ago
>I read that as "one operation per test". 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.
ziml77 5y ago
Your interpretation is a saner way to write tests, but I saw no indication that's what he meant. If it was what he meant, then he should have made it clearer. It's not like he couldn't have added an extra couple pages with an explanation and examples.

Toine 5y ago

These are minor inconveniences relative to the massive benefits all these techniques bring to your codebase. "Go to implementation" shortcut is a thing and solves the first 2. "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) is absolutely fine (even you could do it in 1 assert).

pydry 5y ago
If he meant one "concept" he probably would have said that. 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.

YEwSdObPQT 5y ago

In simple cases many of the suggestions in the clean code book make things more complicated. 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.

jseban 5y ago

I agree, these rules make the code extremely verbose and over abstracted and scattered. The resulting code base will be several times the size of a straight forward solution, and this has a very big negative impact on maintainability. 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.

rightbyte 5y ago

I agree with my whole heart. 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.

BrS96bVxXBLzf5B 5y ago
Could you provide an example of how you would restructure some nested ifs to jumps? I can kind of picture it but can also imagine doing it badly and being in pain.
ziml77 5y ago
These are the points that I've come to despise from Clean Code. After I read that book I wrote code that tried to follow all those rules. It didn't seem bad at the time, but returning to code written that way has always sucked hard. You have to jump around far too much with the 7 line function rule. And over-DRY code can actually be harder to change since repetition is often incidental. In that case a change in one place will actually affect more than its supposed to.

BigJ1211 5y ago

Also if you look at the code samples he provides they often don't follow his own rules. (At least that was the case when I read the book, don't know if it's been revised since)

addicted 5y ago

If you’re just using your classes as data records, most languages have better options for you. 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.

yakubin 5y ago
Unfortunately, Rust doesn't. Rust only has "struct"-s, no classes, and they're private by default. Enums require the "pub" keyword once before a variant name, and once inside tuple associated with the variant. So often it's two "pub" keywords per line in an enum definition. So after writing code for a while, I then, upon compilation, need to fix up tens of "pub" occurrences. Pretty annoying. Wasted time, and the end result is hideous.

cerved 5y ago

> Prefer polymorphism to if/else or switch/case (oh, the joy of tracing a simple task through 50 files) If this is arduous it may be worthwhile to change your dev environment

ziml77 5y ago
I can hit F12 or Ctrl-Click to navigate to definitions and I get a back button to return to where I was across those navigation operations. Yet it still sucks to bounce through all those files. And it's usually not just a matter of drilling down through the calls, but also moving back up to find out how a dependency of a class that I've ended up in is initialized.

barrkel 5y ago

Private - or visibility scoping generally - is absolutely essential when you program in an environment with multiple teams. 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.

yakubin 5y ago
I agree. What I'm saying is that private is a poor default.
- thfuran 5y ago
  Why? I think it's pretty reasonable to only expand APIs deliberately.
  - yakubin 5y ago
    Because most things in healthy programs aren't APIs, but just plain data: records, sum types and composites thereof (arrays, stacks, queues, whatever). 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.

ravenstine 5y ago

How else is "Uncle" Bob Martin supposed to sell more books and presentations if people actually write code they can understand and are happy with? :D

nfw2 5y ago

IMO it's generally good advice to be wary of rolling too many disjoint use cases into one abstraction, and I think that's the rationale behind recommending polymorphism, but the ergonomics of actually writing OO code make it impractical advice to try to follow as a rule. Inversion-of-control is often a more practical way to solve the same problem, in my experience

kgeist 5y ago

>the default case is that you want everything public 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).

yakubin 5y ago
I've partially responded to that already[1][2]. Let me add to that, that when you group together data, a lot of invariants are preserved by the logic you already wrote for the types you compose your types of. Most basic grouping of objects is a struct. It's a product type. Often what I want really is the product of all the states of the component types, without eliminating any combinations. And in the case of sum types: I practically always want all the states. All the invariants are already handled in the types being summed. Not for transfer/storage, but because it logically makes sense. Another thing is that "make invalid state unrepresentable" is a good guideline, but it can lead you into a corner, where you write a lot of code just to make sure that your type system proves that a certain error is not representable by a type, when this error is trivial, unlikely and would take seconds to debug at runtime (maybe would even be so obvious, as to not need any debugging at all). Just because a certain invariant is true in a domain, doesn't mean that it needs to be encoded in the type system. We need to choose what's the most productive use of our time as developers. Those are trade-offs. 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. [1]: <https://news.ycombinator.com/item?id=30112738 https://news.ycombinator.com/item?id=30112738> [2]: <https://news.ycombinator.com/item?id=30113932 https://news.ycombinator.com/item?id=30113932>