Hacker News Reader

Comment by whstl | Hacker News Reader

To quote from Clean Code:

"The function is a bit too long and the variables are used throughout. To split the function into smaller pieces we need to create a GuessStatisticsMessage class and make the three variables fields of this class." - Add Meaningful Context, Page 28

EDIT: And then right below there's an example where the author lifts variables into object state.

EDIT 2: Funny enough, ChatGPT managed to refactor to something even much shorter and IMO much clearer than both examples in the book:

    private void printGuessStatistics(char candidate, int count) {
        if (count == 0) {
            print("There are no " + candidate + "s");
        } else if (count == 1) {
            print("There is 1 " + candidate);
        } else {
            print("There are " + count + " " + candidate + "s");
        }
    }

View on HN

Thank you for doing the work to find that for me.

I still don't see that as:

> "Lifting variables into “global” (object) state"

It is simply the inversion and extraction method that is commonly used. The value of it is lost here as his example is poor IMHO as I find that cleaning up deep nested arrow code is where it is.

This method on page 28 is about refactoring to improve readability, and the location where the variables are declared are still in the same class.

Nothing has changed in that example except adding named private methods in place of logic inside an if-elif-else ladder.

So this:

    if (count == 0) {
       number = "no";
       verb = "are";
       pluralModifier = "s";
       } 
    else if (count == 1) {
       number = "1";
       verb = "is";
       pluralModifier = "";
       }
    else {
       number = Integer.toString(count);
       verb = "are";
       pluralModifier = "s";
       }

Is changed to this:

    if (count == 0) {
       thereAreNoLetters();
       }
    else if (count == 1) {
       thereIsOneLetter();
       }
    else {
       thereAreManyLetters(count);
       }

Remember this chapter is about "name things" not flow control or even data flow.

It actually is intended to help with most of the concerns above and has nothing to do with the react style anti-parameter drilling style of 'lifting'

If you go to page 97 he goes over the 'The Law of Demeter' and argues against exactly what was above and actually cites Martin Fowlers refactoring book which is written in a far better style and tries to call out nuance.

So my opinion that he gives semi-helpful advice that he over sells as received wisdom still holds. Obviously the cost of calling a private method in your language and how that impacts your use case matter here.

> This method on page 28 is about refactoring to improve readability, and the location where the variables are declared are still in the same class.

Same class, but method-scope local variables were unnecessary turned into long-lived instance variables. This is what GP means by "Lifting variables into “global” (object) state". Their phrasing, not mine.

This unnecessary usage of state for what could be a simple variable binding is what GP is criticizing, and they're arguing that is problematic for maintenance and readability. I agree with them.

In that example the constructor parameters were just moved, is there some cost in overloading the empty params?

Original (labeled bad by the book):

    private void printGuessStatistics(char candidate, int count) {
       String number;
       String verb;
       String pluralModifier;

Modified version:

    public class GuessStatisticsMessage {
       private String number;
       private String verb;
       private String pluralModifier;

       public String make(char candidate, int count) {
          createPluralDependentMessageParts(count);
          return String.format(
            "There %s %s %s%s",
            verb, number, candidate, pluralModifier );
       }

Once again, it was presented in the context of naming things....not object or data structures.

Often one has to resort to spherical cows to explain things.

(Edited to add the overloaded constructor params)

> In that example the constructor parameters were just moved

Not really. In the "Original" example there's no constructor in sight, as that's just a standalone procedural-style method, not a class.

The second one turns the method into a class.

> is there some cost

As GP mentions, this incurs mental costs: "In more complex classes, it’s often hard to even identify that this has happened. Is this permanent object state, or just an internal temporary variable that made it easier to leap around methods?"

In terms of performance, this depends on the language/runtime/type, but in general you'll get heap allocation instead of a stack, and it will use the GC.

Also if the lifetime of the private fields is the same of GuessStatisticsMessage, so you'll waste memory if they're complex objects that don't need to live for as long as the class. Depends, YMMV. I once had a memory leak-ish issue due to that in a Ruby app.

EDIT:

> Once again, it was presented in the context of naming things....not object or data structures

This is fine, but the example is not even good in terms of names. `make` is a terrible method name [1], and, turning local variables into long-lived object variables doesn't improve their names, it only muddies the waters like GP mentioned.

[1] Classic parody/criticism of the style here: http://steve-yegge.blogspot.com/2006/03/execution-in-kingdom...

It all doesn't matter, these are code fragments that are not in context of a full program.

Lets look at the labels on these code blocks (in a chapter called "Chapter 2: Meaningful Names")

> Listing 2-1 > Variables with unclear context.

> Listing 2-2 > Variables have a context.

What part of that would even suggest that they are indicating the most performant, idealized code?

They are non-complete random code fragments meant to discuss one of the hardest problems in computer science...naming things.

It is simply a poor example of "Variables have a context" being more readable than "Variables with unclear context", not a suggestion that the lifting principle is required.

This is _simply_ an example that is similar to the extract and inversion method that all of us that get brought in to break up legacy monoliths have to do because often things are a big ball of much with vars that look more like it was written in the language Brainfuck or obfuscated Perl than anything reasonable.

That is not anything that the Clean camp, or even Bob came up with...it is often the reality when trying to save systems from architectural erosion or....

But seriously if you consider 'clean code' as cargo culting design choices without though from any source, you are going to be screwed no matter what.

The number of competing needs related to a dozen types of cohesion along with a dozen types of coupling will never reduce to a simple set of rules.

Strings are interned either in the string pool, or even if you use New...which this code wasn't, anything newer than 8u20 will dedupe and duplicate strings will only have a single instance even if they aren't in the string pool. So long lived strings total space usage isn't really massive.

If you choose to save the size of a pointer with what appears a short lived single computational class anyway, sacrificing readability and maintainability, you will have far greater problems than inefficient memory use.

> not a suggestion that the lifting principle is required.

But there is such suggestion. In the quoted part of the book I sent above, Uncle Bob uses the words we need: https://news.ycombinator.com/item?id=42489167

> This is _simply_ an example

You asked where in the book the idea criticized by GP came from, and I only answered. I'm not trying to argue.

> What part of that would even suggest that they are indicating the most performant, idealized code?

Nowhere. You're the one who asked for "the cost". I'm not in an argument, I'm only answering your questions.

The parameters have moved to a class method, still within the class, which is now public, but still taking a string and an intrinsic int, the string, as being passed as a parameter is already in the string pool or perhaps on the heap outside the string pool if it was created with New,

Nothing has been elevated outside of the block scope.

As private methods don't use the table now:

https://github.com/openjdk/jdk11u-dev/commit/942001236f945c2...

Where is anything that was claimed about raising variables to global scope as was suggested above apply.

You still have a constructor btw, just the make() method overloading the missing class construction.

The parameters are just placeholders, they have to be instantiate when invoked even if the implicit construction is hidden.

But in the beginning example and the refactored example they have the same instance variables, with the latter being marked private but that doesn't impact the lifecycle.

The question being if the refactored version uses the new private methods as callable or if optimization removes them.

The point being is that blaming performance regressions on this type of refactoring to improve readability is on thin ground for java.

This example does nothing to suggest that global state APIs etc.. that are a problem in react relate at all to the recommendation, which are still containing everything in a local scope.

I thank you for taking the time to answer, but what I am looking for is how this somehow lifts vars to a global scope, which it doesn't.

Sure, no problem. Happy to answer questions.

Reply on news.ycombinator.com