Comment by ajuc

I've seen threads on this before but the "goto" (couldn' t stop myself) reaching of object oriented-ness to "solve" everything is really frustrating.

I've found the single greatest contributor to more readable and maintainable code is to limit state as much as possible.

Which was really hard for me to learn because it can be somewhat less efficient, and my game programmer upbringing hates it.

> if they were one function before they probably share some state

and this is exactly why you refactor to pull out the shared state into parameters, so that each of the "subfunctions" have zero side effects.

In javascript I sometimes break up the behaviour of a large function by putting small internal functions inside it. Those internal functions often have side effects, mutating the state of the outer function which contains them.

I find this approach a decent balance between having lots of small functions and having one big function. The result is self contained (like a function). It has the API of a function, and it can be read top to bottom. But you still get many of the readability benefits of small functions - like each of the internal methods can be named, and they’re simple and each one captures a specific thought / action.

Now this is usually in my opinion not a good advice (it is like reintroduction of global variables) as unnecessary state certainly makes things more difficult to reason about.

I have read the book (not very recently) and I do not recall this but perhaps I am just immune to such advice.

I like his book about refactoring more than Clean Code but it introduced me to some good principles like SOLID (a good mnemonic), so I found it somewhat useful.

The problem is abstraction isn't free. Sometimes it frees up your brain from unnecessary details and sometimes the implementation matters or the abstraction leaks.

Even something as simple as Substring which is a method we use all the time and is far more clear than most helper functions I've seen in code bases.

Is it Substring(string, index, length) or Substring(string, indexStart, indexEnd)

What happens when you pass in "abc".Substring(0,4) do you get an exception or "abc"?

What does Substring(0,-1) do? or Substring (-2,-3).

What happens when you call it on null? Sometimes this matters, sometimes it doesn't.

I posted this elsewhere in the thread, but local blocks that define which variables they read, mutate and export would IMO be a very good solution to this problem:

    (reads: var_1, var_2; mutates: var_3) {
       var_3 = var_1 + var_2
       int result_value = var_1 * var_2
    } (exports: result_value)

    return result_value * 5

There are a couple of newer languages experimenting with concepts like this, Jai being one: https://youtu.be/5Nc68IdNKdg?t=3493

> Lots of tiny functions kills readability.

I've heard this argument a lot, and I've found generally there's another problem that causes lack of readability than the small functions.

>Assuming your language supports this. C++ notably doesn't, especially in the cases where you'd produce such small functions - inside a single translation unit, in an anonymous namespace, where enforcing "caller before callee" order would require you to forward-declare everything up front. Which is work, and more lines of code.

Here though you're kind of used to reading code upwards though, so flip the depth first and make it depth last (or take the hit on the forward declarations. If you've got more than you can handle of these, your classes are probably too complex regardless (i.e. doing input, parsing, transformation, and output in the same method).

> quite often a properly descriptive name would be 100+ characters long

Generally if this is the case, then the containing class / module / block / ? is too big. Not a problem of small methods, problem is at a higher level.

> Explicit pre/post conditions in each method

I should have been more explicit here - what I meant is that you know that in the first method, that only the first 3 variables matter, and those variables / parameters are not modified / relevant to the rest of the method. Even without specifically coding pre/post-cons, you get a better feel for the intended isolation of each block. You fall into a pattern of writing code that is simple to reason about. Paired with pure methods / immutable variables, this tends to (IMO) generate easily scannable code. Code that looks like it does what it does, rather than code that requires reading every line to understand.

I have seen so much GUI code like this in my career! Real world sophisticated GUIs can have tens or hundreds of attributes to setup. Especially ancient Xlib stuff, this was the norm. You have a few functions with maybe hundreds of lines doing pure GUI setup. No problem -- easy to mentally compartmentalise.

Your deeper point (if I may theorise): Stop following hard-and-fast rules. Instead, do what makes sense and is easy to read and maintain.

> I know how to do it, I just don't always think it's worth it.

Agreed:)

Generally no problem with this method other than it's difficult to at a glance see what each item will get set to. Something like the following might be an easy first step:

    function getFullParameters() {
      function param(defaultValue, redraws) {
        return { defaultValue: defaultValue, randomFn: null, redraws };
      }
      function param(defaultValue, randomFn, redraws) {
        return { defaultValue: defaultValue, randomFn: randomFn, redraws };
      }
      const result = {
        "gridType": param(1, onlyOneRedraw("grid")),
        "gridSize": param(32, onlyOneRedraw("grid")),
        "gridOpacity": param(40, onlyOneRedraw("grid")),
        "width": param(1024, allRedraws()),
        "height": param(1024, allRedraws()),
        "seed": param(1, () => Math.round(Math.random() * 65536), allRedraws()),
        "treeDensity": param(40, () => Math.round(Math.random() * 100), onlyOneRedraw("trees")),
        "stoneDensity": param(40, () => Math.round(Math.random() * 20 * Math.random() * 5), onlyOneRedraw("stones")),
        "twigsDensity": param(40, () => Math.round(Math.random() * 20 * Math.random() * 5), onlyOneRedraw("twigs")),
        "riverSize": param(3, () => Math.random() > 0.5 ? Math.round(Math.random() * 10) : 0, onlyRedrawsAfter("river")),
        "roadSize": param(0, () => Math.random() > 0.5 ? Math.round(Math.random() * 10) : 0, onlyRedrawsAfter("river")),
        "centerRandomness": param(20, () => Math.round(30), onlyOneRedraw("trees")),
        "leavedTreeProportion": param(95, () => Math.round(Math.random() * 100), onlyOneRedraw("trees")),
        "treeSize": param(50, () => Math.round(30) + Math.round(Math.random() * 40), onlyOneRedraw("trees")),
        "treeColor": param(120, () => Math.round(Math.random() * 65536), onlyOneRedraw("trees")),
        "treeSeparation": param(40, () => Math.round(80 + Math.random() * 20), onlyOneRedraw("trees")),
        "serrationAmplitude": param(130, () => Math.round(80 + Math.random() * 40), onlyOneRedraw("trees")),
        "serrationFrequency": param(30, () => Math.round(80 + Math.random() * 40), onlyOneRedraw("trees")),
        "serrationRandomness": param(250, () => Math.round(100), onlyOneRedraw("trees")),
        "colorRandomness": param(30, () => Math.round(20), onlyOneRedraw("trees")),
        "clearings": param(9, () => Math.round(3 + Math.random() * 10), onlyRedrawsAfter("clearings")),
        "clearingSize": param(30, () => Math.round(30 + Math.random() * 20), onlyRedrawsAfter("clearings")),
        "treeSteps": param(2, () => Math.round(3 + Math.random() * 2), onlyOneRedraw("trees")),
        "backgroundNo": param(1, onlyTheseRedraws(["background", "backgroundCover"])),
        "showColliders": param(0, onlyOneRedraw("colliders")),
        "grassLength": param(85, () => Math.round(25 + Math.random() * 50), onlyTheseRedraws(["background", "backgroundCover"])),
        "grassDensity": param(120, () => Math.round(25 + Math.random() * 50), onlyTheseRedraws(["background", "backgroundCover"])),
        "grassSpread": param(45, () => Math.round(5 + Math.random() * 25), onlyTheseRedraws(["background", "backgroundCover"])),
        "autoredraw": { defaultValue: true, randomFn: null, redraw: noneRedraws(), },
      };
      return result;
    }

For someone looking at this for the first time, the rationale for each random function choice is obtuse so you might consider pulling out each type of random function into something descriptive like randomIntUpto(65536), randomDensity(20, 5), randomIntRange(30, 70).

Does it add value? Maybe - ask a junior to review the two and see which they prefer maintaining. Regardless, this code mostly exists at a single level of abstraction, which tends to imply simple refactorings rather than complex.

My guess is if this extended to multiple (levels / maps / ?) you'd probably split the settings into multiple functions, one per map right...?

Well, inlining by the compiler would be expected but we do not only write the code for the machine but also for another human being (that could be yourself at another moment of time of course).

Splitting the code into smaller functions does not automatically warrant a better design, it is just one heuristic.

A naive implementation of the principle could perhaps have found a less optimal solution

  function positiveInt max(positiveInt value1, positiveInt value2)
    return value1 > value2 ? value1 : value2

  function positiveInt total(positiveInt value1, positiveInt value2)
    return value1 + value2 

  function positiveInt calcMeaningOfLife(positiveInt[] values)
    positiveInt total = 0
    positiveInt max = 0
    for (positiveInt i=0; i < values.length; i++)
      total = total(total, values[i])
      max = max(max, values[i])
    return total - max

Now this is a trivial example but we can imagine that instead of max and total we have some more complex calculations or even calls to some external system (a database, API etc).

When faced with a bug, I would certainly prefer the refactoring in the GP comment than one here (or the initial implementation).

I think that when inlining feels strictly necessary then there has been problem with boundary definition but I agree that being able to view one single execution path inlined can help to understand the implementation.

I completely agree that naming and abstractions are perhaps two most complicated problems.