Jakub is a Java EE developer since 2005 and occasionally a project manager, working currently with Iterate AS. He's highly interested in developer productivity (and tools like Maven and AOP/AspectJ), web frameworks, Java portals, testing and performance and works a lot with IBM technologies. A native to Czech Republic, he lives now in Oslo, Norway. Jakub is a DZone MVB and is not an employee of DZone and has posted 155 posts at DZone. You can read more from them at their website. View Full User Profile

Hidden Dependencies Are Evil – Arguing With The Clean Code (Slightly)

02.21.2011
| 5079 views |
  • submit to reddit

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:

-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?

Then, on line 36, why do we set as the value of the argument marshaler the argument name (i.e. an iterator pointing to it)? It gets clear when we look into the implementation of some ArgumentMarshalers:

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.

 

From http://theholyjava.wordpress.com/2011/02/19/hidden-dependencies-are-evil-arguing-with-the-clean-code/

Published at DZone with permission of Jakub Holý, author and DZone MVB.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Tags:

Comments

Sylvain Brocard replied on Mon, 2011/02/21 - 11:55am

The last version of Args (Listing 14-16) does not have the 'inexplicable code'. It just silently ignore wrong arguments.
private void parseArguments() throws ArgsException {
  for (currentArgument = argsList.iterator(); currentArgument.hasNext();) {
    String arg = currentArgument.next();
    parseArgument(arg);
  }
}

private void parseArgument(String arg) throws ArgsException {
  if (arg.startsWith("-"))
    parseElements(arg);
}

Jakub Holý replied on Tue, 2011/02/22 - 4:14pm in response to: Sylvain Brocard

Rather confusingly the chronologically last version of Args isn't the last listed in the chapter (14-16) but the first one (14-2) for the chapter starts with showing the result and only then going through the refinements from the first til the (last - 1) version.

Michal Huniewicz replied on Thu, 2011/03/31 - 12:07pm

Hi Jakub, I also find the previous(); and break; lines confusing. Surely someone knows how we should look at them?

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.