The problem here is the signature of setLocation
. It's stringly typed.
To elaborate: Why would it expect String
? A String
represents any kind of textual data. It can potentially be anything but a valid location.
In fact, this poses a question: what is a location? How do I know without looking into your code? If it were a URL
than I would know a lot more about what this method expects.
Maybe it would make more sense for it to be a custom class Location
. Ok, I wouldn't know at first, what that is, but at some point (probably before writing this.configuration.getLocation()
I would take a minute to figure out what it is this method returns).
Granted, in both cases I need to look some place else to understand, what is expected. However in the latter case, if I understand, what a Location
is, I can use your API, in the former case, if I understand, what a String
is (which can be expected), I still don't know what your API expects.
In the unlikely scenario, that a location is any kind of textual data I would reinterpret this to any kind of data, that has a textual representation. Given the fact, that Object
has a toString
method, you could go with that, although this demands quite a leap of faith from the clients of your code.
Also you should consider, that this is Java you're talking about, which has very few features by design. That's what's forcing you to actually call the toString
at the end.
If you take C# for example, which is also statically typed, then you would actually be able to omit that call by defining behavior for an implicit cast.
In dynamically typed languages, such as Objective-C, you don't really need the conversion either, because as long as the value behaves like a string, everybody is happy.
One could argue, that the last call to toString
is less a call, than actually just noise generated by Java's demand for explicitness. You're calling a method, that any Java object has, therefore you do not actually encode any knowledge about a "distant unit" and thereby don't violate the Principle of Least Knowledge. There is no way, no matter what getLocation
returns, that it doesn't have a toString
method.
But please, do not use strings, unless they are really the most natural choice (or unless you're using a language, that doesn't even have enums ... been there).
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.
Best Answer
After reading the very interesting article posted by Robert Harvey, I'd suggest that if you're still worried about the LoD you do this (forgive me for not using ObjC syntax here, I have no clue how that works):
Rationale: By introducing additional getters for sub-properties, you're flattening out your domain model, and that's just horrible: you lose all structure, and increase coupling. (What if you add a property to Ad? Do you now go to every single class that embeds an Ad and add getters for the new property?)
However, if you have a hierarchy of objects and a matching (partial) pseudo-hierarchy of view elements to display them, it makes sense to organize your update functions in a hierarchy as well.