Refining Uncle Bob’s Clean Code

I’ve just finished reading ‘Uncle Bob’s’ new book ‘Clean Code‘. I fully agree with most of the statements and it was a pleasure to read, especially because Uncle Bob and his co-authors have a talent for putting some of the most relevant values and principles of software development into so simple words (i wished they have crossed my mind within more than one discussion in the past).

A book about ‘Clean Code‘ wouldn’t be a truly book about code if it wouldn’t contain some code. And yes, this book is full of code, surrounded by some useful ruminations and critical discussions on how to improve the given code – getting your feet wet and looking at some real world examples is just the flesh on the bones for a book about code.

As Uncle Bob encouraged the reader within the introduction of the book to ‘work hard while reading the book‘ in terms of thinking what’s right and what’s wrong about a given piece of code, so did i with his refined Args example at the end of Chapter 14 ‘Successive Refinement‘.

The Boy Scout Rule

Personally, i like the idea of Uncle Bob’s ‘Boy Scout Rule‘ – leaving the campground in a better state than you found it. So looking at a piece of code, you always take care of it, improving it if necessary, so that the normal tendency of code degeneration is interrupted but rather gets better and better over time.

When i first came across the code of the Args example (at the start of the chapter), i honestly wasn’t sure if this was already the refactored version or still the original version before refactoring (it turned out to be the refactored one). Don’t get me wrong, the given code is in really good shape, but for some points i’m not sure if you still can improve readability and structure by applying some of Uncle Bobs principles (given in the book resp. some of the OO principles from his book ‘Agile Software Development‘).

So applying the the Boy Scout Rule to the Args example, the following sections will give some ruminations about the given code, the principles it may violate or miss, along with some suggestions on how to improve it.

Thanks, Uncle Bob !

Like Uncle Bob mentioned when discussing SerialDate (Chapter 16), each programmer shows a big portion of courage when offering his code to the community, abandoning it to discussion and critical review. Like Uncle Bob appreciated those traits to the author of SerialDate, so it is to Uncle Bob. He immediately permits my question for the following review of his Args code and gave accreditation to present his refactored code. Thanks, Oncle Bob!

Clean Code

So without further ado, let’s take a closer look at Uncle Bob’s refined code. I will mainly show you the code of class Args, as it contains most of the logic i’m going to refine:

import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;

public class Args {
  private String schema;

  private Map<Character, ArgumentMarshaler> marshalers =
    new HashMap<Character, ArgumentMarshaler>();
  private Set<Character> argsFound = new HashSet<Character&amp>();
  private Iterator<String> currentArgument;
  private List<String> argsList;

  public Args(String schema, String[] args) throws ArgsException {
    this.schema = schema;
    argsList = Arrays.asList(args);
    parse();
  }

  private void parse() throws ArgsException {
    parseSchema();
    parseArguments();
  }

  private boolean parseSchema() throws ArgsException {
    for (String element : schema.split(",")) {
      if (element.length() > 0) {
        parseSchemaElement(element.trim());
      }
    }
    return true;
  }

  private void parseSchemaElement(String element) throws ArgsException {
    char elementId = element.charAt(0);
    String elementTail = element.substring(1);
    validateSchemaElementId(elementId);
    if (elementTail.length() == 0)
      marshalers.put(elementId, new BooleanArgumentMarshaler());
    else if (elementTail.equals("*"))
      marshalers.put(elementId, new StringArgumentMarshaler());
    else if (elementTail.equals("#"))
      marshalers.put(elementId, new IntegerArgumentMarshaler());
    else if (elementTail.equals("##"))
      marshalers.put(elementId, new DoubleArgumentMarshaler());
    else
      throw new ArgsException(ArgsException.ErrorCode.INVALID_FORMAT, elementId, elementTail);
  }

  private void validateSchemaElementId(char elementId) throws ArgsException {
    if (!Character.isLetter(elementId)) {
      throw new ArgsException(ArgsException.ErrorCode.INVALID_ARGUMENT_NAME, elementId, null);
    }
  }

  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);
  }

  private void parseElements(String arg) throws ArgsException {
    for (int i = 1; i < arg.length(); i++)
      parseElement(arg.charAt(i));
  }

  private void parseElement(char argChar) throws ArgsException {
    if (setArgument(argChar))
      argsFound.add(argChar);
    else {
      throw new ArgsException(ArgsException.ErrorCode.UNEXPECTED_ARGUMENT, argChar, null);
    }
  }

  private boolean setArgument(char argChar) throws ArgsException {
    ArgumentMarshaler m = marshalers.get(argChar);
    if (m == null)
      return false;
    try {
      m.set(currentArgument);
      return true;
    } catch (ArgsException e) {
      e.setErrorArgumentId(argChar);
      throw e;
    }
  }

  public int cardinality() {
    return argsFound.size();
  }

  public String usage() {
    if (schema.length() > 0)
      return "-[" + schema + "]";
    else
      return "";
  }

  public boolean getBoolean(char arg) {
    ArgumentMarshaler am = marshalers.get(arg);
    boolean b = false;
    try {
      b = am != null && (Boolean) am.get();
    } catch (ClassCastException e) {
      b = false;
    }
    return b;
  }

  public String getString(char arg) {
    ArgumentMarshaler am = marshalers.get(arg);
    try {
      return am == null ? "" : (String) am.get();
    } catch (ClassCastException e) {
      return "";
    }
  }

  public int getInt(char arg) {
    ArgumentMarshaler am = marshalers.get(arg);
    try {
      return am == null ? 0 : (Integer) am.get();
    } catch (Exception e) {
      return 0;
    }
  }

  public double getDouble(char arg) {
    ArgumentMarshaler am = marshalers.get(arg);
    try {
      return am == null ? 0 : (Double) am.get();
    } catch (Exception e) {
      return 0.0;
    }
  }

  public boolean has(char arg) {
    return argsFound.contains(arg);
  }
}

Separation of concerns

First of all, i’ve asked myself, why does class Args do so much? If you look at the code you can see at least two separate activities: Parse the given schema (tangled with the Selection of the related ArgumentMarshaler), followed by the iteration of the current arguments, including the determination of the potential argument Ids along with the population of the related argument values (belonging to the given argument id) to the responsible ArgumentMarshaler.

Is there maybe more than one reason to change that class, thus violating the Single Responsibility Principle? For example if you want to extend or change the notation of the schema definition, you surely have to reflect that fact in the parsing strategie of the schema. Similarly, if you want to pass the given arguments in another format (say within a hierarchy, or allowing a sophisticated argument chain), you also have to change the class.

Naming

Those two tasks aren’t closely coupled: The output of parsing the schema (a Set of appropriate ArgumentMarshaler) serves as input for processing the given arguments. So nothing would argue against separating those two tasks by releasing each of them in an own class, say ArgumentPopulator and MarshalersFactory (providing a Number of ArgumentMarshalers for a given schema – we’ll get back to them in a Minute).

Why should i name them this way? First of all, if you look at method parseSchema(), it gives you not to much hints about its effects. Surely, it’s gonna parse the given schema, but it doesn’t say anything about it’s intention, that is to identify and select a set of appropriate ArgumentMarshalers which are capable to handle the given arguments, specified by the schema. So parsing the schema is correct but only half the true, characterizing the pure action. The real intention is to retrieve those ArgumentMarshalers. Therefore it’s best located in a Factory for ArgumentMarshalers (as long as we don’t want to care about the concrete implementations), providing a method getMarshalersFor( schema ) that clearly states its intent (aka its effects).

Same goes for method parseArguments(). Again, its intention isn’t clear by looking at the methods name. We want to browse through the given arguments, that’s clear – but for what reason? Detecting the distinct arguments and passing the related argument values to the associated ArgumentMarshaler! In other words: we want to populate our ArgumentMarshaler with the given argument values – that’s the real intention behind the argument parsing.

Separate Constructing a System from Using it

As stated in chapter 11 ‘Systems‘, some of the famous enterprise frameworks nowadays, advocate the separation of setting up a System from Running the System. The Setup is done for example by inspecting a separate configuration file, where all classes and its dependencies are defined followed by the instantiation of those classes, including the ‘wiring’ of dependend beans (you can see this ‘pattern’ clearly when looking at the Spring framework for example). With this pattern comes Dependency Injection in a more or less normal way: The building block (or main) who’s is responsible for setting up the system is the only one who ‘sees’ all beans, thus can statisfy the needed dependencies of all beans by injecting them.

Looking back at the Args example, i would see class Args as the root of its ‘application’, thus responsible for bringing the different parts into life and statisfying their needs. Under this view, Args is nothing more than a facade which delegates the different tasks to their collaborators:

import static java.util.Arrays.asList;

public class Args {
    ...

    private Marshalers argumentMarshalers = null;

    public Args( String schema, String[] args ) throws ArgsException {
        argumentMarshalers = Marshallers.Factory.getMarshalersFor( schema );
        ArgumentPopulator.distribute( asList( args ) ).to( argumentMarshalers );
      }

     ...

}

As you can see, the result of the schema parsing (that is the found ArgumentMarshalers by Marshallers.Factory, each responsible for every single found argumentId) acts as input for the parsing / population of the arguments. Therefore, this second action only needs to know about the current arguments and the responsible Marshalers which will receive the related argument values they have to convert.

Boundaries

As you may have seen in the above piece of code, we no longer rely on a Map when handling ArgumentMarshalers. Like said in Chapter 8 ‘Boundaries‘, Provider of third party packages ‘strive for broad applicability. Users on the other side are looking for an interface that’s focussed on their particular needs‘.

Further on, Ron Jeffries mentioned in Chapter 1, that most programs include handling collections (the marshalers in our case). He also suggested to come up with an own collection type for specific items, so that you’re able to provide a ‘domain specific’ interface, related to the specific needs of usage.
That said, we could encapsulate the Set of responsible Marshalers in an own collection (say Marshalers), only providing domain specific Methods (like isMarshallerAvailabeFor( String argumentId )), but prohibit the possibility to clear the collection for example:

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class Marshalers {

    private Map<String,ArgumentMarshaler> argumentIdMarshalers = null;

    private Marshalers( List<ArgumentMarshaler> marshalers ){

        this.argumentIdMarshalers = new HashMap<String, ArgumentMarshaler>();

        for( ArgumentMarshaler marshaler : marshalers ){
            argumentIdMarshalers.put( marshaler.getArgumentId(), marshaler );
        }
    }

    public boolean isMarshalerAvailabeFor( String argumentId ){
        return argumentIdMarshalers.containsKey( argumentId );
    }

    public ArgumentMarshaler getMarshalerFor( String argumentId ) throws ArgsException{
        return argumentIdMarshalers.get( argumentId );
    }

    public int size(){
        return argumentIdMarshalers.size();
    }
}

As you can see, java.util.Map is now nothing more than an implementation detail, not showing up at the interface of the Marshalers collection.

Power to the ArgumentMarshaler

One thing that surprised me most, is the if/else if/else chain within method parseSchemaElement() in order to select an appropriate ArgumentMarshaler for a given argumentId (schema element).
Again, i wonder if Args should be responsible to bound a specific argumentId to a specific ArgumentMarshaler, or if this shouldn’t be in the responsibility of the single Marshaler.

I would rather like to have the possibility of asking a Marshaler, if he’s able to handle a given schema element, so that the Marshaler can decide of its own if he wants to accept a given schema element. This would mean, that we could get rid of the if/else if/else chain by only iterating the registered Marshalers until there’s a Marshaler found that feels responsible for the given element (replacing conditional with polymorphism).
Furthermore, parsing and separating the single schema elements (knowledge about the schema notation) and the ‘selection’ of an appropriate Marshaler is cleanly separated.

Since we already moved the accordant logic to Marshalers.Factory, let’s take a closer look at the new mechanism on how to identify an appropriate ArgumentMarshaler for a given schema element:

public class Marshalers {

    ...    

    public static class Factory{

        private static final List<ArgumentMarshaler> registeredMarshalers =
                new ArrayList<ArgumentMarshaler>();

        public static void register( ArgumentMarshaler marshaler ){
            registeredMarshalers.add( marshaler );
        }

        public static Marshalers getMarshalersFor( String schema ) throws ArgsException{
            List<ArgumentMarshaler> marshalers = new ArrayList<ArgumentMarshaler>();

            for( String element : schema.split( "," ) ) {
                  if( element.length() > 0 ) {
                      marshalers.add( marshalerFor( element.trim() ) );
                  }
            }
            return new Marshalers( marshalers );
        }

        private static ArgumentMarshaler marshalerFor( String element ) throws ArgsException {

            for( ArgumentMarshaler marshaler : registeredMarshalers ){            

                if( marshaler.canHandle( element ) ){
                    return marshaler.newInstanceFor( element );
                }
            }
            throw new ArgsException( ArgsException.ErrorCode.INVALID_FORMAT, element, element );
        }
    }
}

As you might see, we shifted some of the responsibility to ArgumentMarshaler. Now each Marshaler knows for himself, if he wants to handle a specific schema element. You might say, that the argument type postfix (like ‘*’ or ‘#’) now is hidden inside of each single Marshaler, but that’s only a question of configuration. As you will see, it would be easy to come up with a slide variation, where the argument type postfix could be passed as a constructor argument to each single marshaler implementation (that kind of configuration would fall in the responsibility of class Args, since it is responsible for the Setup like mentioned before).

Another point worth to mention: Factory is an inner class within Marshalers, so that it’s able to access the private constructor of Marshalers for returning a new instance. Since the constructor is private, the only way to receive an instance of Marshalers is via Marshalers.Factory.getMarshalersFor( String schema ).

But now, let’s take a closer look at the new interface of ArgumentMarshaler:

public interface ArgumentMarshaler {

    public boolean canHandle( String schemaElement );

    public int getNumberOfArgs();

    public ArgumentMarshaler newInstanceFor( String schemaElement );

    public String getArgumentId();

    public void set( String...arguments ) throws ArgsException;

    public Object get();

}

As you can see, an ArgumentMarshaler is now a little bit more powerful. As said, it can decide, wether or not a given schema element can be handled by a distinct Marshaler, it also ‘knows’ about the current argumentId it’s responsible for (so we can get rid of the set ‘argsFound‘, since we only have to ‘ask’ the current Set of Marshalers about the dedicated argumentIds. In addition to that, a Marshaler knows about the number of arguments he’s capable to process. As you will see, there might be some kind of Marshalers, that may take more than one argument for conversion (like for exampe a DateArgumentMarshaler, which will take three arguments – year, month and day).

One of the most important points – each Marshaler acts as a Factory for producing new instances of its own type. We surely could have separated those two responsibilities, but for this first refinement, i’ll leave it within the Marshaler (that’s kind of ugly, i know – but i want to leave some work for the next boy scouts ;o)).

Can you handle this ?

Let’s take a closer look on how a distinct ArgumentMarshaler decides, if he can handle a given schema element. For this feature, i extended the functionality a bit. In the origin example it was only possible to allow argumentIds that consist of a single letter. But there are surely some argumentIds that might be longer than a single character (like -classpath or -keystore).

I decided to use an abstract base class that will process the most part of the decision, leaving only the ‘details’ to the implementations:

import java.util.regex.Pattern;

public abstract class AbstractArgumentMarshaler implements ArgumentMarshaler {

    private String argumentId = null;

    public AbstractArgumentMarshaler( String argumentId ){
        this.argumentId = argumentId;
    }

    @Override
    public String getArgumentId() {
        return argumentId;
    }

    public abstract Pattern getArgumentIdPattern();

    public abstract String getArgumentTypePostfix();

    public abstract ArgumentMarshaler newInstance( String argumentId );

    public boolean canHandle( String element ){

        if( element.length() < getArgumentTypePostfix().length() ){
            return false;
        }

        String argumentId = removeArgumentTypePostfixFrom( element ); 

        return
            element.endsWith( getArgumentTypePostfix() ) &&
            consistsAtLeastOfOneCharacter( argumentId ) &&
            matches( argumentId, getArgumentIdPattern() );
    }

    public ArgumentMarshaler newInstanceFor( String schemaElement ){
        return newInstance( removeArgumentTypePostfixFrom( schemaElement ) );
    }

    private boolean consistsAtLeastOfOneCharacter( String argumentId ){
        return argumentId != null && argumentId.length() > 0;
    }

    private boolean matches( String argumentId, Pattern regexp ){
        return regexp.matcher( argumentId ).matches();
    }

    private String removeArgumentTypePostfixFrom( String element ){
        return element.substring( 0, ( element.length() - getArgumentTypePostfix().length() ) );
    }
}

As you might see, AbstractArgumentMarshaler puts some burden on its subclasses (the mentioned details). It needs to know the argumentTypePostfix (like ‘*’) and the so called argumentIdPattern – a regexp pattern, constraining the possible instantiations of allowed argumentIds. To be compatible with the origin behaviour, we would only allow a single character argumentId ( ‘[a-z]’).

… yes, I can handle this !

When inheriting from AbstractArgumentMarshaler, there’s again not too much more left to do than fulfilling the claimed template methods and processing the passed arguments. Let’s look at some concrete ArgumentMarshaler:

import java.util.Calendar;
import java.util.Date;
import java.util.regex.Pattern;

import com.mgi.util.args.ArgsException.ErrorCode;

public class DateArgumentMarshaler extends AbstractArgumentMarshaler {

	private static final Pattern ARGUMENT_ID_PATTERN = Pattern.compile( "([a-z]|[A-Z])*" );
	private static final String ARGUMENT_TYPE_POSTFIX = "<date>";

	private Date date = null;

	public DateArgumentMarshaler( String argumentId ) {
		super(argumentId);
	}

	public DateArgumentMarshaler(){
		this( null );
	}

	@Override
	public ArgumentMarshaler newInstance(String argumentId) {
		return new DateArgumentMarshaler( argumentId );
	}

	@Override
	public Pattern getArgumentIdPattern() {
		return ARGUMENT_ID_PATTERN;
	}

	@Override
	public String getArgumentTypePostfix() {
		return ARGUMENT_TYPE_POSTFIX;
	}

	@Override
	public int getNumberOfArgs() {
		return 3;
	}

	@Override
	public void set(String... arguments) throws ArgsException {

		assertValid( arguments );

		try{
			Calendar calendar = Calendar.getInstance();
			calendar.set( Calendar.DATE, Integer.parseInt( arguments[0] ) );
			calendar.set( Calendar.MONTH, Integer.parseInt( arguments[1] ) - 1 );
			calendar.set( Calendar.YEAR, Integer.parseInt( arguments[2] ) );
			calendar.set( Calendar.MINUTE, 0 );
			calendar.set( Calendar.SECOND, 0 );
			calendar.set( Calendar.MILLISECOND, 0 );

			this.date = calendar.getTime();
		}
    	catch (NumberFormatException e) {
    		throw new ArgsException( ErrorCode.INVALID_DATE_ELEMENT, getArgumentId(), arguments[0] );
    	}
	}

	private void assertValid( String... arguments ) throws ArgsException {

		if( arguments[0] == null ||
			arguments[1] == null ||
			arguments[2] == null ){

			throw new ArgsException( ErrorCode.MISSING_DATE_ELEMENT, getArgumentId(), null );
		}
	}

	@Override
	public Object get() {
		return date;
	}
}

And another one …

import java.util.regex.Pattern;
import com.mgi.util.args.ArgsException.ErrorCode;

public class IntegerArgumentMarshaler extends AbstractArgumentMarshaler {

	private static final Pattern ARGUMENT_ID_PATTERN = Pattern.compile( "[a-z]" );
	private static final String ARGUMENT_TYPE_POSTFIX = "#";

	private int intValue = 0;

	private IntegerArgumentMarshaler( String argumentId ){
		super( argumentId );
	}

	protected IntegerArgumentMarshaler(){
		this( null );
	}

	@Override
	public ArgumentMarshaler newInstance(String argumentId) {
		return new IntegerArgumentMarshaler( argumentId );
	}

	@Override
	public String getArgumentTypePostfix() {
		return ARGUMENT_TYPE_POSTFIX;
	}

	@Override
	public Pattern getArgumentIdPattern() {
		return ARGUMENT_ID_PATTERN;
	}

	@Override
	public int getNumberOfArgs() {
		return 1;
	}

    public void set(String... arguments) throws ArgsException {

    	if( arguments.length == 0 || arguments[0] == null ){
    		throw new ArgsException( ErrorCode.MISSING_INTEGER, getArgumentId(), null );
    	}

    	try {
    		intValue = Integer.parseInt(arguments[0]);
    	}
    	catch (NumberFormatException e) {
    		throw new ArgsException( ErrorCode.INVALID_INTEGER, getArgumentId(), arguments[0] );
    	}
    }

    public Object get() {
      return intValue;
    }
  }

I think you can see that implementing a new ArgumentMarshaler is not more complicated as before. Just answer some more questions about it’s context – the main part remains quite the same.

Give it to me, Baby …

Now that we know how a single Marshaler works and how we can retrieve them from our Marshalers collection, it’s very easy to pass the current arguments to them. As said before, we just shifted that task into a class of its own – ArgumentPopulator:

import java.util.Iterator;
import java.util.List;

public class ArgumentPopulator {

	private List<String> argsList = null;

	public static ArgumentPopulator distribute( List<String> argsList ){
		return new ArgumentPopulator( argsList );
	}

	private ArgumentPopulator( List<String> argsList ){
		this.argsList = argsList;
	}

	public void to( Marshalers marshalers ) throws ArgsException{

		for ( Iterator<String> arguments = argsList.iterator(); arguments.hasNext(); ) {

			String argument = arguments.next();

			if ( representsArgumentId( argument ) ){

				String argumentId = extractArgumentIdFrom( argument );

				if( ! marshalers.isMarshalerAvailabeFor( argumentId ) ){
					throw new ArgsException( ArgsException.ErrorCode.UNEXPECTED_ARGUMENT, argumentId, null );
				}

				ArgumentMarshaler marshaler = marshallers.getMarshalerFor( argumentId );				  			

				marshaler.set( getArgumentsAhead( marshaller.getNumberOfArgs(), arguments ) );
			}
		}
	}

	private String extractArgumentIdFrom(String argument) {
		return argument.substring( 1, argument.length() );
	}

	private boolean representsArgumentId(String argument) {
		return argument.startsWith( "-" );
	}

	private String[] getArgumentsAhead( int numberOfArgs, Iterator<String> arguments ) {
		  String[] args = new String[numberOfArgs];

		  for( int i=0; i < numberOfArgs; i++ ){
			  args&#91;i&#93; = arguments.hasNext() ? arguments.next() : null;
		  }
		  return args;
	  }
}
&#91;/sourcecode&#93;

As you can see, ArgumentPopulator accepts a List of current arguments (by using the provided Factory method <em>distribute()</em>) and tries to push the found arguments to any collection of passed <em>Marshalers</em> (using the<em> to()</em> method).
<h3>Symmetry of abstraction</h3>
If you look at the original code (like method <span><span><em>parseArgument()</em> and some of its called methods) you might detect a mix of levels of abstraction. For example, deciding if the current argument represents the introduction of a new argumentId (by comparing the first character of the current argument with '-') might be a level below the next command - parsing the element. I'm not sure about that, but i think encapsulating this decision within a method hides the concrete implementation, expressing only the purpose of the inspection of the current argument.</span></span>
<h3><span><span>Open Closed Principle</span></span></h3>
What do we have to do, if we want to add a new Marshaler? As Uncle Bob mentioned in his book, you have to adapt more than one place in the code to integrate a new one. Essentialy you have to alter method <span><span><em>parseSchemaElement()</em>. Personally, i think this method lies on a to detailed implementation level for being a good place to add new Marshalers. You have to come a long path until you end up in this method.</span></span>

<span><span>I think, adding a new Marshaler should be done on the very top level of the 'application', in a very simple way: registering a new Marshaler and that's it - no hassling with implementation details.
Again, if we look at class <em>Args </em>as the root of its 'application', that's the perfect place for constructing the system, and hence registering all available ArgumentMarshalers. Since <em>Marshalers.Factory</em> is under control of <em>Args</em>, it can be 'dependency injected' simply by registering all available ArgumentMarshalers.
</span></span>


public class Args {

	static{
		Marshalers.Factory.register( new BooleanArgumentMarshaler() );
		Marshalers.Factory.register( new IntegerArgumentMarshaler() );
		Marshalers.Factory.register( new StringArgumentMarshaler() );
		Marshalers.Factory.register( new DateArgumentMarshaler() );
	}
        ...
}

I’m not sure if we can speak about violating OCP in the orig code, but now for me it feels more like adding new Behaviour by changing configuration, not by changing implementation.

Another point in relation to OCP is the addition of a new getter method that comes with every new ArgumentMarshaler (eg. getDate() for DateArgumentMarshaler). If you want to avoid that, you could of course come up with a more generic method, using the type token idiom (that may be not type safe) – so let’s complete Args:

public class Args {
        ...
	public <T> T get( String argumentId, Class<T> asType ) throws ArgsException{
		return (T) argumentMarshalers.getMarshalerFor( argumentId ).get();
	}

	public int cardinality() {
		return argumentMarshalers.size();
	}	  

	public boolean has( String argumentId ) {
	    return argumentMarshallers.isMarshalerAvailabeFor( argumentId );
	}

}

Of course, we could have made the get() method more type safe by doing a type check with a following cast and maybe providing a default value of for the claimed type if the client didn’t ‘hit’ the appropriate type for a certain argumentId. But at the end of the day, the client should handle its mistake, so i think it’s better to come up with an exception.

Extinguishing the campfire

As you have seen, there are quite a few changes to Uncle Bob’s orig code. I think at the end it’s the surrounding forces and given values that will drive the final structure of the code.

Of course, the refactored solution has still some obstacles and cornercases – now it’s up to the next boy scout to move on … ;o)

For me, those changes reflected the way i think and try to communicate my intentions through the code. That may not be the way of communication for other developers. In general, it’s worth to take a deeper look into the psychological context of software development when it comes to communication. Moving forward to a common understanding of how to transport intention and meaning between humans might be a key to cleaner code in respect to its readability.