Naresh Jain » Agile FAQs Blog » Managed Chaos

Everything else is just Noise

Tuesday, September 22nd, 2009

Recently I was working on some code. The code was trying to tell me many things, but I was not sure if I was understanding what it was trying to communicate. It just felt irrelevant or noise at that moment. Somehow the right level of abstraction was missing.

When I started I had:

private final UserService userService = createMock(UserService.class);
private final DomainNameService dns = createMock(DomainNameService.class);
private final RandomNumberGenerator randomNumberGenerator = new RandomNumberGenerator() {
    @Override
    public long next() {
        return 9876543210L;
    }
};
private final IdentityGenerator identityGenerator = new IdentityGenerator(randomNumberGenerator, dns, userService);
private final User naresh_from_mumbai = new User("naresh", "jain", "mumbai", "india", "indian");

@Test
public void avoidRestrictedWordsInIds() {
    expect(dns.isCelebrityName("naresh", "jain")).andStubReturn(false);
    expect(dns.validateFirstPartAndReturnRestrictedWordIfAny("naresh")).andStubReturn("naresh");
 
    expect(dns.isCelebrityName("nares", "jain")).andStubReturn(false);
    expect(dns.validateFirstPartAndReturnRestrictedWordIfAny("nares")).andStubReturn(null);
    expect(dns.validateSecondPartAndReturnRestrictedWordIfAny("jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("nares@jain.com")).andStubReturn(true);
 
    expect(dns.isCelebrityName("nares", "india")).andStubReturn(false);
    expect(dns.isCelebrityName("naresh", "india")).andStubReturn(false);
    expect(dns.validateFirstPartAndReturnRestrictedWordIfAny("nares")).andStubReturn(null);
    expect(dns.validateSecondPartAndReturnRestrictedWordIfAny("india")).andStubReturn(null);
    expect(userService.isIdentityAvailable("nares@india.com")).andStubReturn(true);
 
    expect(dns.isCelebrityName("nares", "indian")).andStubReturn(false);
    expect(dns.isCelebrityName("naresh", "indian")).andStubReturn(false);
    expect(dns.validateFirstPartAndReturnRestrictedWordIfAny("nares")).andStubReturn(null);
    expect(dns.validateSecondPartAndReturnRestrictedWordIfAny("indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("nares@indian.com")).andStubReturn(true);
 
    expect(dns.isCelebrityName("nares", "mumbai")).andStubReturn(false);
    expect(dns.isCelebrityName("naresh", "mumbai")).andStubReturn(false);
    expect(dns.validateFirstPartAndReturnRestrictedWordIfAny("nares")).andStubReturn(null);
    expect(dns.validateSecondPartAndReturnRestrictedWordIfAny("mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("nares@mumbai.com")).andStubReturn(true);
 
    replay(userService, dns);
 
    List<String> generatedIDs = identityGenerator.getGeneratedIDs(naresh_from_mumbai);
    List<String> expectedIds = ids("nares@jain.com", "nares@india.com", "nares@indian.com", "nares@mumbai.com");
 
    assertEquals(expectedIds, generatedIDs);
 
    verify(userService, dns);
}

As you can see, my first reaction after looking at this code was that there is too much going on, most of which is duplicate. So cleaned it up a bit and made it more expressive by

private final Context lets = new Context(userService, dns);
 
@Test
public void avoidRestrictedWordsInIds() {
    lets.assume("naresh").plus("jain").isNotACelebrityName();
    lets.assume("naresh").isARestrictedUserName();
 
    lets.assume("nares").plus("jain").isNotACelebrityName();
    lets.assume("nares").isNotARestrictedUserName();
    lets.assume("jain").isNotARestrictedDomainName();
    lets.assume().identity("nares@jain.com").isAvailable();
 
    lets.assume("nares").plus("india").isNotACelebrityName();
    lets.assume("nares").isNotARestrictedUserName();
    lets.assume("india").isNotARestrictedDomainName();
    lets.assume().identity("nares@india.com").isAvailable();
 
    lets.assume("nares").plus("indian").isNotACelebrityName();
    lets.assume("nares").isNotARestrictedUserName();
    lets.assume("indian").isNotARestrictedDomainName();
    lets.assume().identity("nares@indian.com").isAvailable();
 
    lets.assume("nares").plus("mumbai").isNotACelebrityName();
    lets.assume("nares").isNotARestrictedUserName();
    lets.assume("mumbai").isNotARestrictedDomainName();
    lets.assume().identity("nares@mumbai.com").isAvailable();
 
    List<String> generatedIds = suggester.generateIdsFor(naresh_from_mumbai);
 
    lets.assertThat(generatedIds).are("nares@jain.com", "nares@india.com", "nares@indian.com", "nares@mumbai.com");
}

By introducing a new class called Context and moving all the mocking code into that, my test looked lot more clear. I was also able to create an abstraction that could communicate intent much more easily.

Next I reduced the clutter further by creating another level of abstraction as follows

@Test
public void avoidRestrictedWordsInIds() {
    lets.assume("naresh", "jain").isNotACelebrityName();
    lets.assume("naresh").isARestrictedUserName();
 
    for (final String[] identityTokens : list(_("nares", "jain"), _("nares", "india"), _("nares", "indian"), _("nares", "mumbai"))) {
        lets.assume(identityTokens[0], identityTokens[1]).isNotACelebrityName();
        lets.assume(identityTokens[0]).isNotARestrictedUserName();
        lets.assume(identityTokens[1]).isNotARestrictedDomainName();
        lets.assume().identity(identityTokens[0] + "@" + identityTokens[1] + ".com").isAvailable();
    }
 
    List<String> generatedIds = suggester.generateIdsFor(naresh_from_mumbai);
 
    lets.assertThat(generatedIds).are("nares@jain.com", "nares@india.com", "nares@indian.com", "nares@mumbai.com");
}

But at this point, even though the code ended up being very dense, it was very difficult to understand what was going on and why so. In a desperate search for simplicity and better communication, I ended up with

@Test
public void avoidRestrictedWordsInIds() {
    lets.assume("naresh").isARestrictedUserName();
    List<String> generatedIds = suggester.suggestIdsFor(naresh_from_mumbai);
    lets.assertThat(generatedIds).are("nares@jain.com", "nares@india.com", "nares@indian.com", "nares@mumbai.com");
}

What is interesting about this is that I made some simple assumption saying:

every name is not a celebrity name unless specified
every user name is a valid (non-restricted) user name unless specified
every domain name is a valid (non-restricted) domain name unless specified
every identity is available unless specified

All these assumptions are now capture in my Context object and rest of my tests can happily focus on what really matters. I really liked the way this reduced the clutter in my tests without compromising on communication.

Posted in Design, Programming, Testing | Comments

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.

Posted in Agile, Design, Programming, Testing | Comments

Code should Express Intent and NOT Rely on Side-effects

Friday, July 10th, 2009

Consider the following code

    public static <T> String join(Iterator<T> itr, String seperator) {
        StringBuilder sb = new StringBuilder();
        while (itr.hasNext()) {
            if (sb.length() != 0) {
                sb.append(separator);
            }
            sb.append(itr.next().toString());
        }
        return sb.toString();
    }

When I read through this code, checking StringBuffer’s length and then appending a separator does not communicate the intent very well.

4
5
6

            if (sb.length() != 0) {
                sb.append(separator);
            }

Here we are relying on the side-effect (StringBuffer’s length) instead of asking the iterator. So the code does not flow smoothly. Also, now if you want to add a prefix to the joined String this logic might break.

IMHO, following code solves this problem.

    public static <T> String join(Iterator<T> itr, String seperator) {
        StringBuilder sb = new StringBuilder();
        while (itr.hasNext()) {
            sb.append(itr.next().toString());
            if(itr.hasNext()) {
                sb.append(separator);
            }
        }
        return sb.toString();
    }

Posted in Programming | Comments

Communication in Code #1 Priority

Friday, June 26th, 2009

Here is a sample of code (happens to be from a test) which I think is not communicative.

public class ContactNumberTest {
  final String exampleCountryCode1 = "91";
  final String exampleCountryCode2 = "1";
  final String exampleNumber1 = "8012345678";
  final String exampleNumber2 = "2028569635";
 
  ContactNumber phoneNumberA = new ContactNumber(exampleCountryCode1,exampleNumber1);
  ContactNumber phoneNumberB = new ContactNumber(exampleCountryCode1,exampleNumber1);
  ContactNumber phoneNumberC = new ContactNumber(exampleCountryCode2,exampleNumber2);
 
  @Test
  public void testEquals() {
    //Symmetry
    assertTrue(phoneNumberA.equals(phoneNumberB));
    assertTrue(phoneNumberB.equals(phoneNumberA));
 
    //Reflexivity
    assertTrue(phoneNumberA.equals(phoneNumberA));
 
    //Not equals
    assertFalse(phoneNumberA.equals(phoneNumberC));
  }
 
  @Test
  public void testHashcode() {
    assertTrue(phoneNumberA.hashCode() == phoneNumberB.hashCode());
  }
}

I know you must be wondering why in the first place someone is writing tests for equals and hashCode. I agree. Its not required. But lets say someone really needs to.

This is how I would refactor the code to make it more communicative.

public class ContactNumberTest {
  private final String indiaCountryCode = "+91";
  private final String usCountryCode = "+1";
  private final String cellNumber = "8012345678";
  private final String mobileNumber = "2028569635";
 
  private final ContactNumber indianNumber = new ContactNumber(indiaCountryCode, cellNumber);
  private final ContactNumber sameIndianNumber = new ContactNumber(indiaCountryCode, cellNumber);
  private final ContactNumber usNumber = new ContactNumber(usCountryCode, mobileNumber);
  private final Map<contactnumber , String> users = new HashMap</contactnumber><contactnumber , String>();
  private String userName;
 
  @Test
  public void isSymmetrical() {
    assertNotSame(indianNumber, sameIndianNumber);
    assertEquals(indianNumber, sameIndianNumber);
    assertEquals(sameIndianNumber, indianNumber);
  }
 
  @Test
  public void isReflexive() {
    assertEquals(indianNumber, indianNumber);
  }
 
  @Test
  public void isTransitive() {
    ContactNumber newIndianNumber = new ContactNumber(indiaCountryCode, cellNumber);
    assertEquals(indianNumber, sameIndianNumber);
    assertEquals(sameIndianNumber, newIndianNumber);
    assertEquals(newIndianNumber, indianNumber);
  }
 
  @Test
  public void areNotAlwaysEqual() {
    assertFalse(indianNumber.equals(usNumber));
  }
 
  @Test
  public void equalsIsExceptionFree() {
    assertFalse(indianNumber.equals(null));
  }
 
  @Test
  public void isHashFriendly() {
    addUser("Foo").usingKey(indianNumber);
    assertEquals("Foo", users.get(indianNumber));
    addUser("Bar").usingKey(sameIndianNumber);
    assertEquals(1, users.size());
  }
 
  private void usingKey(final ContactNumber number) {
    users.put(number, userName);
  }
 
  private ContactNumberTest addUser(final String userName) {
    this.userName = userName;
    return this;
  }
}
</contactnumber>

Posted in Agile, Design, Programming, Testing | Comments

Why should I bother to learn and practice TDD?

Thursday, May 21st, 2009

My 2 cents, based on personal experience:

With TDD, I’m a lot more confident about my solutions. If I spot a design improvement, I can quickly jump in and fix it with confidence. When given feedback, I’m able to respond to it quickly. I feel I’m in control.
It really helps me keep my design and code minimalistic and clean. No bells and whistles. No buy one get one free combo offers. <Perfection in design is achieved not when there is nothing more to add, but rather when there is nothing more to take away>
Turns out that my code is lot more easier to test. Because its designed to be testable. Lots of people argue that they will write tests after writing the code and it would be more efficient. The biggest problem I find with this approach is that the code ends up being something not readily testable. Then either I’ve to spend time making it testable or I skip testing saying its not possible or not required.
It helps me build a safety net of executable, living, up-to-date specification for my classes and features. Its a great starting point for new team members to understand my software. Tests are a great reference point for someone who wants to use my code.
TDD is a great teacher. It helps me listen to my code and learn from it. It forces me to pay close attention to what is happening. Its easy to spot bad things faster. Its all about feedback and visibility.
TDD forces me to slow down and think. It encourages me to take baby-steps. Sometimes I find people are in such a hurry that they spill mess all over the place. It takes soo much more time and effort to clean up the mess they leave behind.
My tests tend to communicate my design choices much longer after I’m gone.
I massively reduce the amount of time I spend in the debugger or trying to manually test (monkey test) from the UI. When something breaks, I no longer need to crawl through the logs to figure out where things are going wrong. I get pin-pointed feedback.
TDD helps me maintain focus on measurable outcome (producing software that accomplishes a concrete objective). I’m not longer drifting down ratholes.
TDD also helps me reduce the hand-overs between developers and tests and hence the wastage introduced because of all that overhead and context switching.
And so on…

Having said that, TDD alone is not sufficient to achieve the above. At times you need to spike/prototype things. One needs to have (or at least start focusing on building) a good knowledge of Design Principles and Patterns. Its easy to get lost, having a pair can really help.

Again I don’t want to sound dogmatic. I don’t think TDD is the only way to build great software. There are lots of great developers out there, building amazing software without TDD. However I think TDD is a much easier approach to achieve the same.

Posted in Agile, Design, Testing | Comments

Eradicate Duplication; Embrace Communication

Friday, March 13th, 2009

Yesterday, I spent some time cleaning up Acceptance Tests on a project which exposes some REST APIs.

Following is a snippet of one of the tests:

1	Response response = REST_API_call_Using_Wrapper Which_wraps_xml_response_in_a_response_helper_object;

Assert.IsTrue(response.HasHeader);
Assert.IsTrue(response.HasMessageId);
Assert.IsTrue(response.Has("X-SenderIP: " + senderIp));
Assert.IsTrue(response.Has("X-SenderDomain: " + senderDomain));
Assert.IsTrue(response.Has("X-recipientDomain: " + recipientDomain));
Assert.IsTrue(response.Has("X-SPF: " + spfValue));
Assert.IsTrue(response.Has("X-1stClassification: " + firstClassificationResult));
Assert.IsTrue(response.Has("X-2ndClassification: " + secondClassificationResult));
Assert.IsTrue(response.Has("X-3rdClassification: " + thirdClassificationResult));
Assert.IsTrue(response.Has("X-MANUALLY-CLASSIFIED: " + manuallyClassified));

As you can see there is a lot of duplication (Assert.IsTrue is basically noise). It’s also not very clear what the intent of those assert is.

Since Response is a Test Helper class. We thought moving the asserts on the response makes sense. But we also want to make sure the person reading this test understands that we are verifying a bunch of things on the response object.

Since we are using C#, we could do the following using a Delegate.

1	public delegate void ThingsToBeVerified();

public void AssertThat(ThingsToBeVerified codeBlock)
{
  codeBlock();
}

response.AssertThat(
  delegate{
    response.HasHeader;
    response.HasMessageId;
    response.Has("X-SenderIP: " + senderIp);
    response.Has("X-SenderDomain: " + senderDomain);
    response.Has("X-recipientDomain: " + recipientDomain);
    response.Has("X-SPF: " + spfValue);
    response.Has("X-1stClassification: " + firstClassificationResult);
    response.Has("X-2ndClassification: " + secondClassificationResult);
    response.Has("X-3rdClassification: " + thirdClassificationResult);
    response.Has("X-MANUALLY-CLASSIFIED: " + manuallyClassified);
  }
);

Now that we got the asserts out of the way. The following things stand-out as redundant:

The repeating response word
The semicolon at the end of each line
The ‘: ” + ‘ in each Has call

So we got rid of the delegate and used Method Chaining (fluent interfaces) instead. (Other samples of using Fluent Interfaces in Tests)

response.AssertThat.It
                      .HasHeader
                      .HasMessageId
                      .Has("X-SenderIP",senderIp)
                      .Has("X-SenderDomain",senderDomain)
                      .Has("X-recipientDomain", recipientDomain)
                      .Has("X-SPF", spfValue)
                      .Has("X-1stClassification", firstClassificationResult)
                      .Has("X-2ndClassification", secondClassificationResult)
                      .Has("X-3rdClassification", thirdClassificationResult)
                      .Has("X-MANUALLY-CLASSIFIED", manuallyClassified);

Now the Has call and the parentheses looks redundant. One way to eliminate that is by using Operator overloading, something like:

lets.checkThat(response).HasHeader.HasMessageId.Has + "X-SenderIP" = senderIp + "X-SenderDomain" = senderDomain
        + "X-recipientDomain" = recipientDomain + "X-SPF" = spfValue + "X-1stClassification" = firstClassificationResult
        + "X-2ndClassification" = secondClassificationResult + "X-3rdClassification" = thirdClassificationResult + "X-MANUALLY-CLASSIFIED" = manuallyClassified;

We have not implemented this, but technically its possible to do this.

Posted in Agile, Testing | Comments

Agile India.