Refining Uncle Bob’s Clean Code（一）

December 3, 2008 — Mario Gleichmann

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 ofcode, 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 givencode 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 hiscode to the community, abandoning it to discussion and critical review. Like Uncle Bob appreciated those traits to the author ofSerialDate, 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:

001	import java.util.Arrays;

002	import java.util.HashMap;

003	import java.util.HashSet;

004	import java.util.Iterator;

005	import java.util.List;

006	import java.util.Map;

007	import java.util.Set;

008

009	public class Args {

010	private String schema;

011

012	private Map<Character, ArgumentMarshaler> marshalers =

013	new HashMap<Character, ArgumentMarshaler>();

014	private Set<Character> argsFound = new HashSet<Character&amp>();

015	private Iterator<String> currentArgument;

016	private List<String> argsList;

017

018	public Args(String schema, String[] args) throws ArgsException {

019	this.schema = schema;

020	argsList = Arrays.asList(args);

021

parse();

022

}

023

024	private void parse() throws ArgsException {

025	parseSchema();

026	parseArguments();

027

}

028

029	private boolean parseSchema() throws ArgsException {

030	for (String element : schema.split(",")) {

031	if (element.length() > 0) {

032	parseSchemaElement(element.trim());

033

}

034

}

035	return true;

036

}

037

038	private void parseSchemaElement(String element) throws ArgsException {

039	char elementId = element.charAt(0);

040	String elementTail = element.substring(1);

041	validateSchemaElementId(elementId);

042	if (elementTail.length() == 0)

043	marshalers.put(elementId, new BooleanArgumentMarshaler());

044	else if (elementTail.equals("*"))

045	marshalers.put(elementId, new StringArgumentMarshaler());

046	else if (elementTail.equals("#"))

047	marshalers.put(elementId, new IntegerArgumentMarshaler());

048	else if (elementTail.equals("##"))

049	marshalers.put(elementId, new DoubleArgumentMarshaler());

050

else

051	throw new ArgsException(ArgsException.ErrorCode.INVALID_FORMAT, elementId, elementTail);

052

}

053

054	private void validateSchemaElementId(char elementId) throws ArgsException {

055	if (!Character.isLetter(elementId)) {

056	throw new ArgsException(ArgsException.ErrorCode.INVALID_ARGUMENT_NAME, elementId, null);

057

}

058

}

059

060	private void parseArguments() throws ArgsException {

061	for (currentArgument = argsList.iterator(); currentArgument.hasNext();) {

062	String arg = currentArgument.next();

063	parseArgument(arg);

064

}

065

}

066

067	private void parseArgument(String arg) throws ArgsException {

068	if (arg.startsWith("-"))

069	parseElements(arg);

070

}

071

072	private void parseElements(String arg) throws ArgsException {

073	for (int i = 1; i < arg.length(); i++)

074	parseElement(arg.charAt(i));

075

}

076

077	private void parseElement(char argChar) throws ArgsException {

078	if (setArgument(argChar))

079	argsFound.add(argChar);

080

else {

081	throw new ArgsException(ArgsException.ErrorCode.UNEXPECTED_ARGUMENT, argChar, null);

082

}

083

}

084

085	private boolean setArgument(char argChar) throws ArgsException {

086	ArgumentMarshaler m = marshalers.get(argChar);

087	if (m == null)

088	return false;

089

try {

090	m.set(currentArgument);

091	return true;

092	} catch (ArgsException e) {

093	e.setErrorArgumentId(argChar);

094

throw e;

095

}

096

}

097

098	public int cardinality() {

099	return argsFound.size();

100

}

101

102	public String usage() {

103	if (schema.length() > 0)

104	return "-[" + schema + "]";

105

else

106	return "";

107

}

108

109	public boolean getBoolean(char arg) {

110	ArgumentMarshaler am = marshalers.get(arg);

111	boolean b = false;

112

try {

113	b = am != null && (Boolean) am.get();

114	} catch (ClassCastException e) {

115	b = false;

116

}

117

return b;

118

}

119

120	public String getString(char arg) {

121	ArgumentMarshaler am = marshalers.get(arg);

122

try {

123	return am == null ? "" : (String) am.get();

124	} catch (ClassCastException e) {

125	return "";

126

}

127

}

128

129	public int getInt(char arg) {

130	ArgumentMarshaler am = marshalers.get(arg);

131

try {

132	return am == null ? 0 : (Integer) am.get();

133	} catch (Exception e) {

134

return 0;

135

}

136

}

137

138	public double getDouble(char arg) {

139	ArgumentMarshaler am = marshalers.get(arg);

140

try {

141	return am == null ? 0 : (Double) am.get();

142	} catch (Exception e) {

143	return 0.0;

144

}

145

}

146

147	public boolean has(char arg) {

148	return argsFound.contains(arg);

149

}

150

}

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.