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.
Best Answer
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
Vectorclass with attributesPoint aandPoint b, where the only methods arescaleBy(double factor)andprintTo(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 attributesa,bandc, while the rest usecandd- 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.