Hacker News Reader

Comment by yakubin | Hacker News Reader

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

View on HN

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

This is where Python's "consenting adults" idea comes in. Why the hell should someone change a vector "mid-flight" - that is a huge code smell and should be dealt with in the review / design / discussion / pub.

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.

In most situations, no you can not trust fellow devs to do the right thing and if you push for those reviews then you're the asshole and lose the office politics game. So yeah, I'd rather guardrail people into good behavior.

Can I clarify? You get a review that just looks wrong and you become the asshole? What kind of reviews are these ?

please ignore the above. I am sure reviews can become political arenas

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.

In c++ you choose the default so I'm not sure it should be lumped in with the others.

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.

In c++ a class declared with the keyword 'class' is private by default, and a class declared with the keyword 'struct' is public by default. So if you want public by default just write 'struct' (somehow this is not commonly known).

https://en.wikipedia.org/wiki/C%2B%2B_classes

I think the “somehow” is not too hard to guess… it is easy to think that “class” is the only way to declare a class, especially since “struct” is taken from C. I’m sure very few intro C++ explanations ever mention “struct” as a way to declare a class.

There's quite a lot about C++ which wouldn't be covered in a simple tutorial, but the meaning of class/struct is a pretty basic part of the language, hardly obscure.

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.

This comment represents some of the worst elitism I see in the programming community.

Yes, I meant common introductory material should mention (and use) it (I think Bjarne does mention it, but I wouldnt call it introductory material)

struct over class then.

I guess that for a lot of people it is the case where you have a class with mutable state that is the anomaly.

It's nuts. It's deeply naive and utterly unworkable.

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

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.

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

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

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.

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.

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.

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

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.

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.

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

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>

[2]: <https://news.ycombinator.com/item?id=30113932>

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.

I agree. What I'm saying is that private is a poor default.

Why? I think it's pretty reasonable to only expand APIs deliberately.

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.

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

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.

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

    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.

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.

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

And then you get a release mode warning about maybe unused c, unless you stick c in your code.

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)

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

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.

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.

> 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

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.

Reply on news.ycombinator.com