XNSIO
  About   Slides   Home  

 
Managed Chaos
Naresh Jain's Random Thoughts on Software Development and Adventure Sports
     
`
 
RSS Feed
Recent Thoughts
Tags
Recent Comments

Embracing Simplicity to Kill Conditional Complexity – A Real World Example

Monday, October 28th, 2013

In the Agile India Submission system, we had a feature which would accept different url path to show matching proposals.

For example:
http://present.agileindia.org/agile-india-2014/agile-lifecycle/45_mins –> will return all 45 mins proposals under the Agile-Lifecycle track for the Agile India 2014 Conference.
http://present.agileindia.org/agile-india-2014/beyond-agile/90_mins –> will return all 90 mins proposals under the Beyond-Agile track for the Agile India 2014 Conference.

If we remove the last part .i.e. http://present.agileindia.org/agile-india-2014/agile-lifecycle –> will return all proposals under the Agile-Lifecycle track for the Agile India 2014 Conference.
and http://present.agileindia.org/agile-india-2014 –> will return all proposals for the Agile India 2014 Conference (could be other conferences as well.)

To achieve this, we had the following routes defined:

//URL Path = /agile-india-2014
app\get("/{conference}", function($req) {
    $query_params = structure_request_params($req['matches']);
    ...
});
 
//URL Path = /agile-india-2014/agile-lifecycle
app\get("/{conference}/{track}", function($req) {
    $query_params = structure_request_params($req['matches']);
    ...
});
 
//URL Path = /agile-india-2014/agile-lifecycle/45_mins
app\get("/{conference}/{track}/{label}", function($req) {
    $query_params = structure_request_params($req['matches']);
    ...
});

In our database we had the following:

CREATE TABLE `conference` (
  `key` VARCHAR(255) NOT NULL,
   ...
  PRIMARY KEY  (`key`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
CREATE TABLE `track` (
  `conference` VARCHAR(255) NOT NULL,
  `key` VARCHAR(255) NOT NULL,
  ...
  PRIMARY KEY  (`key`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;  
 
CREATE TABLE `label` (
  `conference` VARCHAR(255) NOT NULL,
  `track` VARCHAR(255) NOT NULL,
  `key` VARCHAR(255) NOT NULL,
  ...
  PRIMARY KEY  (`conference`,`track`,`key`)
) ENGINE= InnoDB DEFAULT CHARSET=utf8;
 
INSERT INTO `conference` VALUES ('agile-india-2014', ...);
INSERT INTO `track` VALUES ('agile-india-2014', 'agile-lifecycle', ...);
INSERT INTO `label` VALUES ('agile-india-2014', 'agile-lifecycle', '45_mins', ...);

And some really complicated logic:

// For example URL Path /agile-india-2014 will result in 
// $req_params = array('conference'=>'agile-india-2014')
// And URL Path /agile-india-2014/agile-lifecycle will result in 
// $req_params = array('conference'=>'agile-india-2014', 'track'=>'agile-lifecycle')
// And URL Path /agile-india-2014/agile-lifecycle/45_mins will result in 
// $req_params = array('conference'=>'agile-india-2014', 'track'=>'agile-lifecycle', 'label'=>'45_mins')
function structure_request_params($req_params){
    $query_params = array();
    if(isset($req_params['label'])){
        if(Conference::exists($req_params['conference'])){
            $query_params['conference'] = $req_params['conference'];
            if(Track::exists(array('conference' => $req_params['conference'], 'key' => $req_params['track']))){
                $query_params['track'] = $req_params['track'];
                if(Label::exists(array(
                                       'conference' => $req_params['conference'], 
                                       'track' => $req_params['track'], 
                                       'key' => $req_params['label'])
                                       )) {
                    $query_params['label'] = $req_params['label'];
                }
                else{
                    $query_params['search_term'] = array($req_params['label']);
                }
            }
            else{
                $query_params['search_term'] = array($req_params['track'], $req_params['label']);
            }
        }
        else{
            $query_params['search_term'] = array($req_params['conference'], $req_params['track'], $req_params['label']);
        }
    }
    else{
        if(isset($req_params['track'])){
            if(Conference::exists($req_params['conference'])){
                $query_params['conference'] = $req_params['conference'];
                if(Track::exists(array('conference' => $req_params['conference'], 'key' => $req_params['track']))){
                    $query_params['track'] = $req_params['track'];
                }
                else{
                    $query_params['search_term'] = array($req_params['track']);
                }
            }
            else{
                if(Track::exists(array('key' => $req_params['conference']))){
                    $query_params['track'] = $req_params['conference'];
                    if(Label::exists(array('track' => $req_params['conference'], 'key' => $req_params['track']))){
                        $query_params['label'] = $req_params['track'];
                    }
                    else{
                        $query_params['search_term'] = array($req_params['track']);
                    }
                }
                else{
                    $query_params['search_term'] = array($req_params['conference'], $req_params['track']);
                }
            }
        }
        else{
            if(isset($req_params['conference'])){
                if(Conference::exists($req_params['conference'])){
                    $query_params['conference'] = $req_params['conference'];
                }
                elseif(Track::exists(array('key' => $req_params['conference']))){
                    $query_params['track'] = $req_params['conference'];
                }
                elseif(Label::exists(array('key' => $req_params['conference']))){
                    $query_params['label'] = $req_params['conference'];
                }
                else{
                    $query_params['search_term'] = array($req_params['conference']);
                }
            }
        }
    }
    return $query_params;
}

And supporting model classes:

class Conference{
    static function exists($conference){
        DB::query('SELECT * FROM conference WHERE conference.key = %s', $conference);
        return (DB::count() > 0);
    }
}
 
class Track{
    static function exists($query_params){
        $search_query = Track::create_search_query($query_params);
        DB::query("SELECT * FROM track WHERE $search_query");
        return (DB::count() > 0);
    } 
 
	private static function create_search_query($query_params){
        $search_query = array();
        foreach($query_params as $key => $value){
            $search_query[] = "$key='$value'";
        }
        return join(' AND ', $search_query);
    }
}
 
class Label{
    static function exists($query_params){
        $search_query = Label::create_search_query($query_params);
        DB::query("SELECT * FROM label WHERE $search_query");
        return (DB::count() > 0);
    }
 
	private static function create_search_query($query_params){
        $search_query = array();
        if(!empty($query_params)){
            foreach($query_params as $key => $value){
                $search_query[] = "$key='$value'";
            }
            return join(' AND ', $search_query);
        }
        return '';
    }
}

This was extremely hard to understand, so I decided to clean it up and get rid of the conditional complexity code smell.

Refactored code:

function structure_request_params($req_params){
    $criteria = array('conference', 'track', 'label');
    $params = array_values($req_params);
    return build_search_query_params($criteria, $params, array());
}
 
function build_search_query_params($criteria, $params, $results) {
    if(empty($params)) return $results;
    if(empty($criteria)) {
        if(!empty($params)) {
            $results['search_term'] = $params;
        }
        return $results;
    }
    if(is_present_in_db($criteria[0], $params[0])) {
        $results[$criteria[0]] = $params[0];
        array_shift($params);
    }
    array_shift($criteria);
    return map_criteria_params($criteria, $params, $results);
}
 
function is_present_in_db($table_name, $value){
    return (DB::queryFirstField("SELECT count(*) FROM $table_name WHERE `key` = %s", $value) > 0);
}

Also with this, we were able to delete the Model classes as they were not required.

Duplicate Code and Ceremony in Java

Thursday, July 21st, 2011

How would you kill this duplication in a strongly typed, static language like Java?

private int calculateAveragePreviousPercentageComplete() {
    int result = 0;
    for (StudentActivityByAlbum activity : activities)
        result += activity.getPreviousPercentageCompleted();
    return result / activities.size();
}
 
private int calculateAverageCurrentPercentageComplete() {
    int result = 0;
    for (StudentActivityByAlbum activity : activities)
        result += activity.getPercentageCompleted();
    return result / activities.size();
}
 
private int calculateAverageProgressPercentage() {
    int result = 0;
    for (StudentActivityByAlbum activity : activities)
        result += activity.getProgressPercentage();
    return result / activities.size();
}

Here is my horrible solution:

private int calculateAveragePreviousPercentageComplete() {
    return new Average(activities) {
        public int value(StudentActivityByAlbum activity) {
            return activity.getPreviousPercentageCompleted();
        }
    }.result;
}
 
private int calculateAverageCurrentPercentageComplete() {
    return new Average(activities) {
        public int value(StudentActivityByAlbum activity) {
            return activity.getPercentageCompleted();
        }
    }.result;
}
 
private int calculateAverageProgressPercentage() {
    return new Average(activities) {
        public int value(StudentActivityByAlbum activity) {
            return activity.getProgressPercentage();
        }
    }.result;
}
 
private static abstract class Average {
    public int result;
 
    public Average(List<StudentActivityByAlbum> activities) {
        int total = 0;
        for (StudentActivityByAlbum activity : activities)
            total += value(activity);
        result = total / activities.size();
    }
 
    protected abstract int value(StudentActivityByAlbum activity);
}

if this were Ruby

@activities.inject(0.0){ |total, activity| total + activity.previous_percentage_completed? } / @activities.size
@activities.inject(0.0){ |total, activity| total + activity.percentage_completed? } / @activities.size
@activities.inject(0.0){ |total, activity| total + activity.progress_percentage? } / @activities.size

or even something more kewler

average_of :previous_percentage_completed?
average_of :percentage_completed?
average_of :progress_percentage?
 
def average_of(message)
	@activities.inject(0.0){ |total, activity| total + activity.send message } / @activities.size
end

An Example of Primitive Obsession

Tuesday, October 20th, 2009

Couple of years ago, I was working with a team which was building an application for handling User profile. It had the following  functionality

  • User Signup
  • User Profile
  • Login/Logout API
  • Authentication and Authorization API
  • and so on…

As you can see, this is pretty common to most applications.

The central entity in this application is User. And we had a UserService to expose its API. So far, so good.

The UserService had 2 main methods that I want to focus on. The first one is:

    public boolean authenticate(String userId, String password) {
        ...
    }

Even before the authenticate() method gets called, the calling class does basic validations on the password parameter. Stuff like

  • the password cannot be null,
  • it needs to be more than 6 char long and less than 30 char long
  • should contain at least one special char or upper case letter
  • should contain at least one letter
  • and so on …

Some of these checks happen to reside as separate methods on a PasswordUtil class and some on the StringUtil class.

Once the authenticate method is called, we retrieve the respective User from the database, fetch the password stored in the database and match the new password against it. Wait a sec, we don’t store plain password in the DB any more, we hash them before we store ’em. And as you might already know, we use one-way hash; which means given a hash, we cannot get back the original string. So we hash the newly entered password. For which we use a HashUtil class. Then we compare the 2 hashes.

The second method is:

    public User create(final UserDTO userDTO) {
       ...
    }

Before the create() method is called, we validate all the fields inside the UserDTO. During this validation, we do the exact same validations on password as we do before the authenticate method. If all the fields are valid, then inside the create method, we make sure no one else has the same userid. Then we take the raw text password and hash it, so that we can store it in our DB. Once we save the user data in DB, we send out an activation email and off we are.

Sorry that was long. What is the point? Exactly my point. What is the point. Why do I need to know all this stuff? I can’t really explain you the pain I go through when I see:

  • All these hops & jumps around these large meaningless classes (UserDTO, PasswordUtil, StringUtil, HashUtil)
  • Conceptual and Data duplication in multiple places
  • Difficulty in knowing where I can find some logic (password logic seems to be sprayed all over the place)
  • And so on …

This is an example of Primitive Obsession.

  • A huge amount of complexity can be reduced,
  • clarity can be increased and
  • duplication can be avoided in this code

If we can create a Password class. To think about it, Password is really an entity like User in this domain.

  • Password class’ constructor can do the validations for you.
  • You can give it another password and ask if they match. This will hide all the hashing and rehashing logic from you
  • You can kill all those 3 Utils classes (PasswordUtil, StringUtil, HashUtil) & move the logic in the Password class where it belong

So once we are done, we have the following method signatures:

    public User userWithMatching(UserId id, Password userEnteredPwd) {
        ...
    }
    public User create(final User newUser) {
       ...
    }

Biggest Stinkers

Monday, October 19th, 2009

At the SDTConf 2009, Corey Haines & I hosted a session called Biggest Stinkers. During this session we were trying to answer the following two (different) questions:

  • As an experienced developer, looking back, what do you think is the stinkiest code smell that has hurt you the most? In other words, which is the single code smell if you go after eradicating, *most* of the design problems in your code would be solved?
  • There are so many different principles and guidelines to help you achieve a good design. For new developers where do they start? Which is the one code smell or principle that we can teach new developers that will help them the most as far as good design goes (other than years of experience)?

Even though the 2 questions look similar, I think the second question is more broader than the first and quite different.

Anyway, this was probably the most crowded session. We had some great contenders for Smelliest Code Smell (big stinker):

We all agreed that Don’t write code (write new code only when everything else fails) is the single most important lesson every developer needs to learn. The amount of duplicate, crappy code (across projects) that exists today is overwhelming. In a lot of cases developers don’t even bother to look around. They just want to write code. This is what measuring productivity & performance based on Lines of Code (LoC) has done to us. IMHO good developers are 20x faster than average developers coz they think of reuse at a whole different level. Some people confuse this guideline with “Not Invented Here Syndrome“. Personally I think NIHS is very important for advancement in our field. Its important to bring innovation. NIHS is at the design & approach level. Joel has an interesting blog post called In Defense of Not-Invented-Here Syndrome.

Anyway, if we agree that we really need to write code, then what is the one thing you will watch out for? SRP and Connascence are pretty much helping you achieve high Cohesion. If one does not have high cohesion, it might be easy to spot duplication (at least conceptual duplication) or you’ll find that pulling out a right abstraction can solve the problem. So it really leaves Duplicate Code and Primitive Obsession in the race.

Based on my experience, I would argue that I’ve seen code which does not have much duplication but its very difficult to understand what’s going on. Hence I claim, “only if the code had better abstractions it would be a lot easier to understand and evolve the code”. Also when you try to eliminate duplicate code, at one level, there is no literal code duplication, but there is conceptual duplication and creating a high order abstraction is an effective way to solve the problem. Hence I conclude that looking back, Primitive Obsession is at the crux of poor design. a.k.a Biggest Stinker.

Refactoring Teaser IV Solution

Sunday, September 27th, 2009

Its been a while since the Fourth Refactoring Teaser was posted. So far, I think this is one of the trickiest refactorings I’ve tried. Refactored half of the solution and rewrote the rest of it.

Particularly thrilled about shrinkage in the code base. Getting rid of all those convoluted Strategies and Child Strategies with 2 main classes was real fun (and difficult as well).  Even though the solution is not up to the mark, its come a long long way from where it was.

Ended up renaming IdentityGenerator to EmailSuggester. Renamed the PartialAcceptanceTest to EmailSuggesterTest. Also really like how that test looks now:

28
29
30
private final User naresh_from_mumbai = new User("naresh", "jains", "mumbai", "india", "indian");
private final Context lets = new Context(userService, dns);
private final EmailSuggester suggester = new EmailSuggester(userService, dns, randomNumberGenerator);
32
33
34
35
36
@Test
public void suggestIdsUsingNameLocationAndNationality() {
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}
38
39
40
41
42
43
@Test
public void avoidRestrictedWordsInIds() {
    lets.assume("naresh").isARestrictedUserName();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}
45
46
47
48
49
50
@Test
public void avoidCelebrityNamesInGeneratedIds() {
    lets.assume("naresh", "jains").isACelebrityName();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}
52
53
54
55
56
57
@Test
public void appendCurrentYearWithFirstNameIfIdIsNotAvailable() {
    lets.assume().identity("[email protected]").isNotAvailable();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}

EmailSuggester’s optionsFor() method turned out to be fairly straightforward.

26
27
28
29
30
31
32
33
34
public List<String> optionsFor(final User user) {
    List<String> ids = new ArrayList<String>();
    List<String> variations = asList(user.lastName, user.countryName, user.countryMoniker, user.city);
    for (String variation : variations) {
        UserData data = new UserData(user.firstName, variation, user.lastName);
        data.addGeneratedIdTo(ids);
    }
    return ids;
}

This method uses UserData class’ addGeneratedIdTo() method to add an email id to the list of ids passed in.

47
48
49
50
51
52
53
54
55
private void addGeneratedIdTo(final List<String> ids) {
    for (EmailData potential : buildAllPotentialEmailCombinations()) {
        String email = Email.create(potential.userName, potential.domain, dns);
        if (userService.isEmailAvailable(email)) {
            ids.add(email);
            break;
        }
    }
}

This method fetches all potential email address combination based on user data as follows:

57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
private List<EmailData> getAllPotentialEmailCombinations() {
    return new ArrayList<EmailData>() {
        {
            add(new EmailData(firstName, seed));
 
            if (seed != lastName) {
                add(new EmailData((firstName + lastName), seed));
                add(new EmailData((firstName + lastName.charAt(0)), seed));
            }
 
            add(new EmailData((firstName + currentYear()), seed));
 
            if (seed != lastName)
                add(new EmailData((firstName + lastName.charAt(0) + currentYear()), seed));
 
            for (int i = 0; i < MAX_RETRIES_FOR_RANDOM_NUMBER; ++i)
                add(new EmailData((firstName + randomNumber.next()), seed));
        }
    };
}

I’m not happy with this method. This is the roughest part of this code. All the

if (seed != lastName) {

seems dodgy. But at least all of it is in one place instead of being scattered around 10 different classes with tons of duplicate code.

For each potential email data, we try to create an email address, if its available, we add it, else we move to the next potential email data, till we exhaust the list.

Given two tokens (user name and domain name), the Email class tries to creates an email address without Restricted Words and Celebrity Names in it.

30
31
32
33
34
35
private String buildIdWithoutRestrictedWordsAndCelebrityNames() {
    Email current = this;
    if (isCelebrityName())
        current = trimLastCharacter();
    return buildIdWithoutRestrictedWordsAndCelebrityNames(current, 1);
}
37
38
39
40
41
42
43
44
45
46
private String buildIdWithoutRestrictedWordsAndCelebrityNames(final Email last, final int count) {
    if (count == MAX_ATTEMPTS)
        throw new IllegalStateException("Exceeded the Max number of tries");
    String userName = findClosestNonRestrictiveWord(last.userName, RestrictedUserNames, 0);
    String domainName = findClosestNonRestrictiveWord(last.domainName, RestrictedDomainNames, 0);
    Email id = new Email(userName, domainName, dns);
    if (!id.isCelebrityName())
        return id.asString();
    return buildIdWithoutRestrictedWordsAndCelebrityNames(id.trimLastCharacter(), count + 1);
}

Influenced by Functional Programming, I’ve tried to use Tail recursion and Immutable objects here.

Also to get rid of massive duplication in code, I had to introduce a new Interface and 2 anonymous inner classes.

5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
public interface RestrictedWords {
    RestrictedWords RestrictedUserNames = new RestrictedWords() {
        @Override
        public boolean contains(final String word, final DomainNameService dns) {
            return dns.isRestrictedUserName(word);
        }
    };
 
    RestrictedWords RestrictedDomainNames = new RestrictedWords() {
        @Override
        public boolean contains(final String word, final DomainNameService dns) {
            return dns.isRestrictedDomainName(word);
        }
    };
 
    boolean contains(final String word, DomainNameService dns);
}

This should give you a decent idea of what the code does and how it does what it does. To check in detail, download the complete project source code.

Also I would recommend you check out some the comparison of code before and after.

Refactoring Teaser IV – Part 2

Tuesday, August 18th, 2009

Time to take the next baby step.

Lets draw our attention to:

public class IDTokens extends ChildStrategyParam {
 
    public IDTokens(final String token1, final String token2) {
        super(token1, token2, null);
    }
 
    @Override
    public String getToken3() {
        throw new UnsupportedOperationException();
    }
}

This code is quite interesting. It suffers with 3 code smells:

  • Black Sheep
  • Refused Bequest
  • Dumb Data Holder

Also this class violates the “Tell don’t Ask” principle.

Then we look at who is constructing this class, and turns out that we have this deadly SuggestionsUtil class (love the name). This class suffers with various code smells:

  • Blatant Duplicate Code
  • Primitive Obsession
  • Switch Smell
  • Conditional Complexity
  • Null Checks
  • Long method
  • Inappropriate Naming

And now the code:

public class SuggestionsUtil {
    private static int MAX_ATTEMPTS = 5;
    private final DomainNameService domainNameService;
 
    public SuggestionsUtil(final DomainNameService domainNameService) {
        this.domainNameService = domainNameService;
    }
public IDTokens getIdentityTokens(String token1, String token2) {
    if (isCelebrityName(token1, token2)) {
        token1 = token1.substring(0, token1.length() - 1);
        token2 = token2.substring(0, token2.length() - 1);
    }
    int loopCounter = 1;
    do {
        loopCounter++;
        String generatedFirstToken = generateFirstToken(token1);
        String generatedSecondToken = generateSecondToken(token2);
        if (generatedFirstToken == null || generatedSecondToken == null)
            return null;
        else if (isCelebrityName(generatedFirstToken, generatedSecondToken)) {
            token1 = generatedFirstToken.substring(0, generatedFirstToken.length() - 1);
            token2 = generatedSecondToken.substring(0, generatedSecondToken.length() - 1);
        } else
            return new IDTokens(generatedFirstToken, generatedSecondToken);
    } while (loopCounter != MAX_ATTEMPTS);
 
    return null;
}
private String generateSecondToken(String token2) {
    int loopCounter = 0;
    String restrictedWord = null;
    do {
        restrictedWord = domainNameService.validateSecondPartAndReturnRestrictedWordIfAny(token2);
        String replacement = null;
        if (restrictedWord != null) {
            replacement = restrictedWord.substring(0, restrictedWord.length() - 1);
            token2 = token2.replaceAll(restrictedWord, replacement);
            loopCounter++;
        }
    } while (restrictedWord != null &amp;&amp; loopCounter != MAX_ATTEMPTS);
 
    if (loopCounter == MAX_ATTEMPTS)
        return null;
    return token2;
}
private String generateFirstToken(String token1) {
 
    int loopCounter = 0;
    String restrictedWord = null;
    do {
        restrictedWord = domainNameService.validateFirstPartAndReturnRestrictedWordIfAny(token1);
        String replacement = null;
        if (restrictedWord != null) {
            replacement = restrictedWord.substring(0, restrictedWord.length() - 1);
            token1 = token1.replaceAll(restrictedWord, replacement);
            loopCounter++;
        }
    } while (restrictedWord != null &amp;&amp; loopCounter != MAX_ATTEMPTS);
 
    if (loopCounter == MAX_ATTEMPTS)
        return null;
    return token1;
}
private boolean isCelebrityName(final String token1, final String token2) {
    return domainNameService.isCelebrityName(token1, token2);
}
 
public String appendTokensForId(final String token1, final String token2) {
    return token1.toLowerCase().concat("@").concat(token2.toLowerCase()).concat(".com");
}

Also have a look at SuggesitonsUtilsTest, it has a lot of Duplication and vague tests. Guess this will keep you busy for then next couple of hours.

Download the Source Code here: Java or C#.

Refactoring Teaser 1: Take 1

Tuesday, July 14th, 2009

Last week I posted a small code snippet for refactoring under the heading Refactoring Teaser.

In this post I’ll try to show step by step how I would try to refactor this mud ball.

First and foremost cleaned up the tests to communicate the intent. Also notice I’ve changed the test class name to ContentTest instead of StringUtilTest, which means anything and everything.

public class ContentTest {
    private Content helloWorldJava = new Content("Hello World Java");
    private Content helloWorld = new Content("Hello World!");
 
    @Test
    public void ignoreContentSmallerThan3Words() {
        assertEquals("", helloWorld.toString());
    }
 
    @Test
    public void buildOneTwoAndThreeWordPhrasesFromContent() {
        assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", helloWorldJava.toPhrases(6));
    }
 
    @Test
    public void numberOfOutputPhrasesAreConfigurable() {
        assertEquals("'Hello'", helloWorldJava.toPhrases(1));
        assertEquals("'Hello', 'World', 'Java', 'Hello World'", helloWorldJava.toPhrases(4));
    }
 
    @Test
    public void returnsAllPhrasesUptoTheNumberSpecified() {
        assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", helloWorldJava.toPhrases(10));
    }
}

Next, I created a class called Content, instead of StringUtil. Content is a first-class domain object. Also notice, no more side-effect intense statics.

public class Content {
    private static final String BLANK_OUTPUT = "";
    private static final String SPACE = " ";
    private static final String DELIMITER = "', '";
    private static final String SINGLE_QUOTE = "'";
    private static final int MIN_NO_WORDS = 2;
    private static final Pattern ON_WHITESPACES = Pattern.compile("\\p{Z}|\\p{P}");
    private List phrases = new ArrayList();
 
    public Content(final String content) {
        String[] tokens = ON_WHITESPACES.split(content);
        if (tokens.length &gt; MIN_NO_WORDS) {
            buildAllPhrasesUptoThreeWordsFrom(tokens);
        }
    }
 
    @Override
    public String toString() {
        return toPhrases(Integer.MAX_VALUE);
    }
 
    public String toPhrases(final int userRequestedSize) {
        if (phrases.isEmpty()) {
            return BLANK_OUTPUT;
        }
        List requiredPhrases = phrases.subList(0, numberOfPhrasesRequired(userRequestedSize));
        return withInQuotes(join(requiredPhrases, DELIMITER));
    }
 
    private String withInQuotes(final String phrases) {
        return SINGLE_QUOTE + phrases + SINGLE_QUOTE;
    }
 
    private int numberOfPhrasesRequired(final int userRequestedSize) {
        return userRequestedSize &gt; phrases.size() ? phrases.size() : userRequestedSize;
    }
 
    private void buildAllPhrasesUptoThreeWordsFrom(final String[] words) {
        buildSingleWordPhrases(words);
        buildDoubleWordPhrases(words);
        buildTripleWordPhrases(words);
    }
 
    private void buildSingleWordPhrases(final String[] words) {
        for (int i = 0; i &lt; words.length; ++i) {
            phrases.add(words[i]);
        }
    }
 
    private void buildDoubleWordPhrases(final String[] words) {
        for (int i = 0; i &lt; words.length - 1; ++i) {
            phrases.add(words[i] + SPACE + words[i + 1]);
        }
    }
 
    private void buildTripleWordPhrases(final String[] words) {
        for (int i = 0; i &lt; words.length - 2; ++i) {
            phrases.add(words[i] + SPACE + words[i + 1] + SPACE + words[i + 2]);
        }
    }
}

This was a big step forward, but not good enough. Next I focused on the following code:

    private void buildAllPhrasesUptoThreeWordsFrom(final String[] words) {
        buildSingleWordPhrases(words);
        buildDoubleWordPhrases(words);
        buildTripleWordPhrases(words);
    }
 
    private void buildSingleWordPhrases(final String[] words) {
        for (int i = 0; i &lt; words.length; ++i) {
            phrases.add(words[i]);
        }
    }
 
    private void buildDoubleWordPhrases(final String[] words) {
        for (int i = 0; i &lt; words.length - 1; ++i) {
            phrases.add(words[i] + SPACE + words[i + 1]);
        }
    }
 
    private void buildTripleWordPhrases(final String[] words) {
        for (int i = 0; i &lt; words.length - 2; ++i) {
            phrases.add(words[i] + SPACE + words[i + 1] + SPACE + words[i + 2]);
        }
    }

The above code violates the Open-Closed Principle (pdf). It also smells of duplication. Created a somewhat generic method to kill the duplication.

    private void buildAllPhrasesUptoThreeWordsFrom(final String[] fromWords) {
        buildPhrasesOf(ONE_WORD, fromWords);
        buildPhrasesOf(TWO_WORDS, fromWords);
        buildPhrasesOf(THREE_WORDS, fromWords);
    }
 
    private void buildPhrasesOf(final int phraseLength, final String[] tokens) {
        for (int i = 0; i &lt;= tokens.length - phraseLength; ++i) {
            String phrase = phraseAt(i, tokens, phraseLength);
            phrases.add(phrase);
        }
    }
 
    private String phraseAt(final int currentIndex, final String[] tokens, final int phraseLength) {
        StringBuilder phrase = new StringBuilder(tokens[currentIndex]);
        for (int i = 1; i &lt; phraseLength; i++) {
            phrase.append(SPACE + tokens[currentIndex + i]);
        }
        return phrase.toString();
    }

Now I had a feeling that my Content class was doing too much and also suffered from the primitive obsession code smell. Looked like a concept/abstraction (class) was dying to be called out. So created a Words class as an inner class.

    private class Words {
        private String[] tokens;
        private static final String SPACE = " ";
 
        Words(final String content) {
            tokens = ON_WHITESPACES.split(content);
        }
 
        boolean has(final int minNoWords) {
            return tokens.length &gt; minNoWords;
        }
 
        List phrasesOf(final int length) {
            List phrases = new ArrayList();
            for (int i = 0; i &lt;= tokens.length - length; ++i) {
                String phrase = phraseAt(i, length);
                phrases.add(phrase);
            }
            return phrases;
        }
 
        private String phraseAt(final int index, final int length) {
            StringBuilder phrase = new StringBuilder(tokens[index]);
            for (int i = 1; i &lt; length; i++) {
                phrase.append(SPACE + tokens[index + i]);
            }
            return phrase.toString();
        }
    }

In the constructor of the Content class we instantiate a Words class as follows:

    public Content(final String content) {
        Words words = new Words(content);
        if (words.has(MIN_NO_WORDS)) {
            phrases.addAll(words.phrasesOf(ONE_WORD));
            phrases.addAll(words.phrasesOf(TWO_WORDS));
            phrases.addAll(words.phrasesOf(THREE_WORDS));
        }
    }

Even though this code communicates well, there is duplication and noise that can be removed without compromising on the communication.

     phrases.addAll(words.phrasesOf(ONE_WORD, TWO_WORDS, THREE_WORDS));

There are few more version after this, but I think this should give you an idea about the direction I’m heading.

Brett’s Refactoring Exercise Solution Take 1

Saturday, June 13th, 2009

Recently Brett Schuchert from Object Mentor has started posting code snippets on his blog and inviting people to refactor it. Similar to the Daily Refactoring Teaser that I’m conducting at Directi. (I’m planning to make it public soon).

Following is my refactored solution (take 1) to the poorly written code.

Wrote some Acceptance Test to understand how the RpnCalculator works:

Then I updated the perform method to

@Deprecated
public void perform(final String operatorName) {
  perform(operators.get(operatorName));
}
 
public void perform(final Operator operator) {
  operator.eval(stack);
  currentMode = Mode.inserting;
}

Notice I’ve deprecated the old method which takes String. I want to kill primitive obsession at its root.

Had to temporarily add the following map (this should go away once our deprecated method is knocked off).

private static Map operators = new HashMap() {
  {
    put("+", Operator.ADD);
    put("-", Operator.SUBTRACT);
    put("!", Operator.FACTORIAL);
  }
 
  @Override
  public Operator get(final Object key) {
    if (!super.containsKey(key)) {
      throw new MathOperatorNotFoundException();
    }
    return super.get(key);
  }
};

Defined various Operators

private static abstract class Operator {
  private static final Operator ADD = new BinaryOperator() {
    @Override
    protected int eval(final int op1, final int op2) {
      return op1 + op2;
    }
  };
 
  private static final Operator SUBTRACT = new BinaryOperator() {
    @Override
    protected int eval(final int op1, final int op2) {
      return op2 - op1;
    }
  };
 
  private static final Operator FACTORIAL = new UnaryOperator() {
    @Override
    protected int eval(final int op1) {
      int result = 1;
      int currentOperandValue = op1;
      while (currentOperandValue &gt; 1) {
        result *= currentOperandValue;
        --currentOperandValue;
      }
      return result;
    }
  };
 
  public abstract void eval(final OperandStack stack);
}

Declared two types of Operators (BinaryOperator and UnaryOperator) to avoid duplication and to make it easy for adding new operators.

public static abstract class BinaryOperator extends Operator {
  @Override
  public void eval(final OperandStack s) {
    s.push(eval(s.pop(), s.pop()));
  }
 
  protected abstract int eval(int op1, int op2);
}

and

public static abstract class UnaryOperator extends Operator {
  @Override
  public void eval(final OperandStack s) {
    s.push(eval(s.pop()));
  }
 
  protected abstract int eval(int op1);
}
    Licensed under
Creative Commons License