Clean code: consequences of short methods with few parameters

Question

Recently during a code review I came across code, written by a new colleague, which contains a pattern with a smell. I suspect that my colleague's decisions are based on rules proposed by the famous Clean Code book (and perhaps by other similar books as well).

It is my understanding that the class constructor is entirely responsible for the creation of a valid object and that its main task is the assignment of an object's (private) properties. It could of course occur that optional property values may be set by methods other than the class constructor, but such situations are rather rare (although not necessarily wrong, provided that the rest of the class takes into account such a property's optionality). This is important, because it allows to ensure that the object is always in a valid state.

However, in the code that I encountered, most property values are actually set by other methods than the constructor. Values that result from calculations are assigned to properties to be used inside several private methods throughout the class. The author seemingly uses class properties as if they were global variables that should be accessible throughout the class, instead of parameterizing these values to the functions that need them. Additionally, the methods of the class should be called in a specific order, because the class won't do much otherwise.

I suspect that this code has been inspired by the advice to keep methods short (<=5 lines of code), to avoid large parameter lists (<3 parameters) and that constructors must not do work (such as performing a calculation of some sort that is essential for the validity of the object).

Now of course I could make a case against this pattern if I can prove that all kinds of undefined errors potentially arise when methods are not called in a specific order. However, I predict that the response to this is going to be adding validations which verify that properties must be set once methods are called that need those properties to be set.

I would however rather propose to completely change the code, so that the class becomes a blue print to an actual object, rather than a series of methods which should be called (procedurally) in a specific order.

I feel that the code that I encountered smells. In fact, I believe there exists a rather clear distinction as to when to save a value in a class property and when to put it into a parameter for a different method to use - I don't really believe they can be alternatives to one another. I am looking for the words for this distinction.

Answer 1

As someone who has read Clean Code and watched the Clean Coders series, multiple times, and often teach and coach other people in writing cleaner code, I can indeed vouch that your observations are correct - the metrics you point out are all mentioned in the book.

However, the book goes on to make other points which should also be applied alongside the guidelines that you pointed out. These were seemingly ignored in the code you're dealing with. This may have happened because your colleague is still in the learning phase, in which case, as much as it is necessary to point out the smells of their code, it's good to remember that they're doing it in good will, learning and trying to write better code.

Clean Code does propose that methods should be short, with as few arguments possible. But along those guidelines, it proposes that we must folow SOLID principles, increase cohesion and reduce the coupling.

The S in SOLID stands for Single Responsibility Principle, which states that an object should be responsible for only one thing. "Thing" is not a very precise term, so the descriptions of this principle vary wildly. However, Uncle Bob, the author of Clean Code, is also the person who coined this principle, describing it as: "Gather together the things that change for the same reasons. Separate those things that change for different reasons." He goes on to say what he means with reasons to change here and here (a longer explanation here would be too much). If this principle was applied to the class you're dealing with, it is very likely that the pieces that deal with calculations would be separated from those that deal with holding state, by splitting the class in two or more, depending on how many reasons to change those calculations have.

Also, Clean classes should be cohesive, meaning that most of its methods use most of its attributes. As such, a maximally cohesive class is one where all methods use all of its attribute; as an example, in a graphical app you may have a Vector class with attributes Point a and Point b, where the only methods are scaleBy(double factor) and printTo(Canvas canvas), both operating on both attributes. In contrast, a minimally cohesive class is one where each attribute is used in one method only, and never more than one attribute is used by each method. In average, a class presents non-cohesive "groups" of cohesive parts - i.e. a few methods use attributes a, b and c, while the rest use c and d - meaning that if we split the class in two, we end up with two cohesive objects.

Finally, Clean classes should reduce coupling as much as possible. While there are many types of coupling worth discussing here, it seems the code at hand mainly suffers from temporal coupling, where, as you pointed out, the object's methods will only work as expected when they're called in the correct order. And like the two guidelines above-mentioned, the solutions to this usually involve splitting the class in two or more, cohesive objects. The splitting strategy in this case usually involves patterns like Builder or Factory, and in highly complex cases, State-Machines.

The TL;DR: The Clean Code guidelines that your colleague followed are good, but only when also following the remaining principles, practices and patterns mentioned by the book. The Clean version of the "class" you're seeing would be split into multiple classes, each with a single responsibility, cohesive methods and no temporal coupling. This is the context where small methods and little-to-no arguments make sense.

Answer 2

It is my understanding that the class constructor is entirely responsible for the creation of a valid object and that its main task is the assignment of an object's (private) properties.

It's usually responsible for putting the object into an initial valid state, yes; other properties or methods may then change the state to another valid state.

However, in the code that I encountered, most property values are actually set by other methods than the constructor. Values that result from calculations are assigned to properties to be used inside several private methods throughout the class. The author seemingly uses class properties as if they were global variables that should be accessible throughout the class, instead of parameterizing these values to the functions that need them. Additionally, the methods of the class should be called in a specific order, because the class won't do much otherwise.

As well as the problems with readability and maintainability that you allude to, it sounds like there are multiple stages of data flow/transform going on within the class itself, which may indicate that the class is falling foul of the single responsibility principle.

suspect that this code has been inspired by the advice to keep methods short (<=5 lines of code), to avoid large parameter lists (<3 parameters) and that constructors must not do work (such as performing a calculation of some sort that is essential for the validity of the object).

Following some coding guidelines whilst ignoring others often does lead to silly code. If we want to avoid a constructor doing work, for example, the sensible way would usually be to do the work before construction and pass the result of that work in to the constructor. (One argument for that approach might be that you're avoiding giving your class two responsibilities : the work of its initialisation, and its 'main job', whatever that is.)

In my experience, making classes and methods small is rarely something i have to bear in mind as a separate consideration - rather, it follows somewhat naturally from single responsibility.

I would however rather propose to completely change the code, so that the class becomes a blue print to an actual object, rather than a series of methods which should be called (procedurally) in a specific order.

You would probably be correct to do so. There's nothing wrong with writing straightforward procedural code; there is a problem in abusing the OO paradigm to write obfuscated procedural code.

I believe there exists a rather clear distinction as to when to save a value in a class property and when to put it into a parameter for a different method to use - I don't really believe they can be alternatives to one another. I am looking for the words for this distinction.

Usually, you shouldn't be putting a value in a field as a way of passing it from one method to another; a value in a field should be a meaningful part of the state of an object at a given time. (I can think of some valid exceptions, but not ones where such methods are public or where there is an order dependency that a user of the class should be aware of)

Answer 3

You're most likely railing against the wrong pattern here. Small functions and low arity are rarely problematic on their own. The real issue here is the coupling that causes order dependence between functions, so look for ways to address that without throwing away the benefits of the small functions.

Code speaks louder than words. People receive these sorts of corrections a lot better if you can actually do part of the refactoring and show the improvement, perhaps as a pair programming exercise. When I do this, I often find it's more difficult than I thought to get the design right, balancing all the criteria.