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

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.