Hidden Dependencies Are Evil - Arguing With The Clean Code (Slightly)
Hidden dependencies are evil because two pieces of code influencing invisibly each other make it very hard to understand what the code is doing. There is an example of an unresolved hidden dependency in the presumabely perfected code in Clean Code's chatper 14: Successive Refinement. When I wrote the first draft of this post I thought I´d be arguing with the author on this point but after reading the next chapter (15) I found him propagating the very same idea. Of course no code is ever perfect but anyway I believe this is something that should have been improved. The final code actually looks really well, it's short, clean, and expressive, but there is this one thing that really troubles me for I find it difficult to understand (and thus quite "unclean"), and that is the method parseArgumentStrings. Perhaps I'm not smart enough but clean code should be dummy-proof anyway :-). The problem is caused by a hidden dependency between methods of the main class Args and between this class and another one, which is modifying Args' internal state variable.
The Args class is supposed to pass command-line arguments for later retrieval. An example input:
- you can see here two arguments with values (integer, string) and three boolean flags condensed into one argument.
You can see that StringArgumentMarshaler itself retrieves the next argument, i.e. the value following the argument name, while the boolean argument marshaler (which expects no value) sets itself to true based just on the option name being present.
As I've said, the problem is caused by hidden dependencies and data interactions.
I of course don't mean you should stop using instance variables but care should be taken that their use and changes are clear at a first glance.
I've therefore wrapped the iterator into a class providing methods with names communicating the intent and pass it to the two parsing methods to make the dependency clear:
And the iterator:
There is actually quite similar reasoning in CC chapter 15: JUnit internals, p. 259:
I've been little daring, correcting Uncle Bob's code, but let me excuse myself with his own words (p. 265):
Allow me one last quote from Clean Code:
The Args class is supposed to pass command-line arguments for later retrieval. An example input:
-n 42 -s "hello!" -bcd- you can see here two arguments with values (integer, string) and three boolean flags condensed into one argument.
The relevant code
public class Args {
// ...
private ListIterator currentArgument;
// ...
public Args(String schema, String[] args) throws ArgsException {
// ...
parseArgumentStrings(Arrays.asList(args));
}
private void parseArgumentStrings(List argsList)
throws ArgsException {
for (currentArgument = argsList.listIterator(); currentArgument
.hasNext();) {
String argString = currentArgument.next();
if (argString.startsWith("-")) {
parseArgumentCharacters(argString.substring(1));
} else {
currentArgument.previous();
break;
}
}
}
private void parseArgumentCharacters(String argChars) throws ArgsException {
for (int i = 0; i < argChars.length(); i++)
parseArgumentCharacter(argChars.charAt(i));
}
private void parseArgumentCharacter(char argChar) throws ArgsException {
ArgumentMarshaler m = marshalers.get(argChar);
if (m == null) {
throw new ArgsException(UNEXPECTED_ARGUMENT, argChar, null);
} else {
argsFound.add(argChar);
try {
m.set(currentArgument);
} catch (ArgsException e) {
e.setErrorArgumentId(argChar);
throw e;
}
}
}
// ...
}
The unclarity and its (partial) explanation
I can easily understand that parseArgumentStrings goes through all the individual arguments, calling parseArgumentCharacters to process each of them (whether a single-letter argument or a "condensed" one). What i find really confusing is the call to currentArgument.previous() followed by break to finish processing of the arguments prematurely:- * Why is the processing finished when an argument not starting with - is encountered? If it is an error situation, shouldn't we rather throw an exception? If it isn't an error situation, why do we ignore the rest of the input??
- How does a non-boolean argument get its value if we only accept arguments starting with a dash?
public class BooleanArgumentMarshaler implements ArgumentMarshaler {
private boolean booleanValue = false;
public void set(Iterator currentArgument) throws ArgsException {
booleanValue = true;
}
}
public class StringArgumentMarshaler implements ArgumentMarshaler {
private String stringValue = "";
public void set(Iterator currentArgument) throws ArgsException {
try {
stringValue = currentArgument.next();
} catch (NoSuchElementException e) {
throw new ArgsException(MISSING_STRING);
}
}
}
You can see that StringArgumentMarshaler itself retrieves the next argument, i.e. the value following the argument name, while the boolean argument marshaler (which expects no value) sets itself to true based just on the option name being present.
The problem
Do you see the problem? I had to look into another classes to understand Args' code. (And I still do not understand the call to previous() and break).As I've said, the problem is caused by hidden dependencies and data interactions.
Sin one: A hidden dependency between methods
parseArgumentCharacter(char argChar) depends on the state of the this.currentArgument iterator - but this fact is obscured both by its signature (it only takes a character) and by its name, which suggests that it only operates on a single character. Thus a dumb reader like me has no clue that he should expect such a side-effect from the method.I of course don't mean you should stop using instance variables but care should be taken that their use and changes are clear at a first glance.
Sin two: External class changing secretly a private state variable
Even worse, some of the ArgumentMarshallers, namely those requiring an input, advance Args' internal currentArgument pointer. How am I supposed to guess that some evil outsider is touching my class' internal variables?!Proposed solution
I really don't like hidden dependencies like these ones because they make understanding code so much harder for simple people like me. I always prefer to make them explicit even if it means adding a parameter to a method or creating a new (ideally immutable, passed there and back) class for encapsulation of the state.I've therefore wrapped the iterator into a class providing methods with names communicating the intent and pass it to the two parsing methods to make the dependency clear:
//...
private void parseArgumentStrings(List argsList)
throws ArgsException {
ArgumentIterator argumentIterator = new ArgumentIterator(argsList);
String argString;
while ((argString = argumentIterator.getNextUnprocessedOrNull()) != null) {
if (argString.startsWith("-")) {
parseArgumentCharacters(
argString.substring(1), argumentIterator);
} else {
// The old inexplicable code; shouldn't it rather throw an
// exception?
argumentIterator.rollbackToPrevious();
break;
}
}
}
private void parseArgumentCharacters(String argChars, ArgumentIterator argumentIterator) throws ArgsException {
for (int i = 0; i < argChars.length(); i++)
parseArgument(argChars.charAt(i), argumentIterator);
}
private void parseArgument(char argChar, ArgumentIterator argumentIterator) throws ArgsException {
ArgumentMarshaler m = marshalers.get(argChar);
if (m == null) {
throw new ArgsException(UNEXPECTED_ARGUMENT, argChar, null);
} else {
argsFound.add(argChar);
if (m.isValueRequired()) {
m.set(argumentIterator.getValueForArgumentOrFail(argChar));
}
}
}
And the iterator:
public class ArgumentIterator {
private ListIterator currentArgument;
public ArgumentIterator(List argsList) {
currentArgument = argsList.listIterator();
}
public String getNextUnprocessedOrNull() {
if (currentArgument.hasNext())
return currentArgument.next();
else
return null;
}
public void rollbackToPrevious() {
currentArgument.previous();
}
public String getValueForArgumentOrFail(char argChar) throws ArgsException {
try {
return currentArgument.next();
} catch (NoSuchElementException e) {
throw new ArgsException(MISSING_ARGUMENT_VALUE, argChar);
}
}
}
There is actually quite similar reasoning in CC chapter 15: JUnit internals, p. 259:
Careful inspection of findCommonSuffix exposes a hidden temporal coupling [G31]; it depends on the fact that prefixIndex is calculated by findCommonPrefix. If these two functions were called out of order, there would be a difficult debugging session ahead. So, to expose this temporal coupling, let’s have findCommonSuffix take the prefixIndex as an argument.Of course I do not claim the code is perfect, there certainly is yet lot of space for further refinements but I believe that it is now much easier to understand.
Conclusion
Hidden dependencies are a bad thing. I firmly believe it's always better to try to make them explicit even if it means less clean code according to other guidelines (such as minimizing the number of method parameters - [F1]).I've been little daring, correcting Uncle Bob's code, but let me excuse myself with his own words (p. 265):
The authors had done an excellent job with it. But no module is immune from improvement, and each of us has the responsibility to leave the code a little better than we found it.It is clear that code quality really requires continual refinement and that there is always something to improve. We must therefore know when is the right time to stop and move on to another task, it should be neither too early, leaving mess behind, nor too late, leaving the customer without the new features he desired. And whenever we come back to an old code, we should follow the Boy Scoute Rule of software craftmanship.
Allow me one last quote from Clean Code:
Often one refactoring leads to another that leads to the undoing of the first. Refactoring is an iterative process full of trial and error, inevitably converging on something that we feel is worthy of a professional.