XNSIO
  About   Slides   Home  
 
Managed Chaos
Naresh Jain's Random Thoughts on Software Development and Adventure Sports
      
`
 
RSS Feed
Recent Thoughts
Tags
Agile agile bengaluru 2010 Agile Coach Camp agileindia agile india agileindia2012 Agile India 2012 AgileIndia2013 Agile India 2013 Agile India 2013 Workshops agileindia2014 Agile India 2014 agile mumbai 2010 ASCI Attendees Profile Code Coverage code smells Conference Continuous Deployment Continuous Integration cyclomatic complexity Design duplicate code Evolutionary Design Extreme Programming Industrial Logic Lean Lean Startup LinkedIn Open Source pair programming post modern agile primitive obsession smell Refactoring Refactoring Teaser Scrum SDTConf simple design TDD technical debt Test Driven Development Testing unit testing user stories video
Recent Comments

When Should We Encourage Developers to Write Comments?

Sunday, May 15th, 2011

Many people will argue that there is more badly written code than good code. And its important to write comments to avoid these situations. Therefore we should encourage (force) people to write comments.

IMHO they are absolutely right that today many project suffer from poorly written code without any (good) comments. However every team I know, that suffers from this problem, has always been told (forced) to write comments. In spite of the emphasis on writing comments it has not really helped them.

I usually ask:

By asking developers to write comments are we really addressing the root of the problem?

.i.e. developers don’t invest quality time to write self-documenting code; code that clearly communicates its intent and does not require the deodorant of comments.

May be its time to try something different?

I have seen this myself many times, when we emphasize & educate the team on how to write clean code and ask them to stop wasting time writing comments, the code starts to communicate lot better. Its lot more maintainable. Also we have found that writing automated tests is a great way to document your intent as well.

This is how I would explain the concept Comments Smell to a team:

Writing comments that explain “how” or “what” the code does, what it does, is evil IMHO. Comments (esp. about what and how) is a clear failure to express the intent in code. Comment is a deodorant to hide that failure (smell).

  • If developers don’t invest time to write clear code, what is the guarantee that they will write clear comments?
  • Is doing a mediocre job at both (comments and code) better than doing a great job at just Code?
  • Will it actually be more productive to do both or just one?

Remember the biggest problem with comments it that they fall out-of-sync with code very soon. So its not just about the extra investment to write good comments, but also the investment to maintain them.

One has to think hard to write code that expresses intent rather than write some sloppy code with poor abstractions and get away (washing their hands off) by writing comments. Developers have to take responsibility for writing code that others can easily understand.

Having said that, there are times when “the why” (why we are doing something in the code, a particular way) is not apparent by just looking at the code. So if we don’t find a suitable way to communicate “the why” through code, comment is the fall back option.

Note that comments are a fall back option in “the why” case rather than a default option.

Posted in Agile, Code Smells, Programming, Training | 3 Comments »

Embracing Context Objects with Fluent Interfaces for my Tests

Thursday, September 24th, 2009

Of late I’ve been toying around with a new way of using Fluent Interfaces with a Context Object for my Tests. Esp. when I’m using Mockito.

In this post (Fluent Interfaces improve readability of my Tests), I’ve taken an example and demonstrated how I’ve evolved my tests to be more expressive. In my quest for getting my tests to communicate precisely to-the-point by hiding everything else which is noise, I’ve stared exploring another way of using Fluent Interfaces.

Following is an example:

1
2
3
4
5
6
7
8
9

@Test
public void redirectSubDomainsPermanently() {
    lets.assume("google.com").getsRedirectedTo("google.in").withSubDomain();
    response = domainForwardingServer.process(requestFor("blog.google.com"));
    lets.assertThat(response).contains(StatusCode.PermanentRedirect)
                             .location("google.in/blog").protocol("HTTP/1.1")
                             .connectionStatus("close").contentType("text/html")
                             .serverName("Directi Server 2.0");
}

lets and on are both Context objects which provide fluent, domain specific api to make the test very easy to read (communicative and expressive). It also helps me hide all my mocking/stubbing related code.

If you compare this with the original code, you can get a sense of what I’m talking about:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

@Test
public void redirectSubDomainsPermanently()  {
    when(request.hostName()).thenReturn("blog.google.com");
    when(request.protocol()).thenReturn("HTTP/1.1");
    when(request.path()).thenReturn("/");
    domain.setDomain("blog.google.com");
    domain.subDomainForwarding(true);
    domain.setForward("google.in");
    response = domainForwardingServer.processMessage(request);
    assertStatus(StatusCode.PermanentRedirect);
    assertLocation("google.in/blog");
    assertProtocol("HTTP/1.1");
    assertConnectionStatusIs("close");
    assertContentType("text/html");
    assertServerName("Directi Server 2.0");
}

Another example showing the Context object and Fluent Interface style is

1
2
3
4
5
6

@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]");
}

As I said, I’m still toying around with this idea. If this works well, may be it will be part of some mocking framework soon.

Posted in Design, Programming, Testing | 1 Comment »

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:

1
2
3
4
5
6
7
8
9
10

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");
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37

@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("[email protected]")).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("[email protected]")).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("[email protected]")).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("[email protected]")).andStubReturn(true);
 
    replay(userService, dns);
 
    List<String> generatedIDs = identityGenerator.getGeneratedIDs(naresh_from_mumbai);
    List<String> expectedIds = ids("[email protected]", "[email protected]", "[email protected]", "[email protected]");
 
    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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31

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("[email protected]").isAvailable();
 
    lets.assume("nares").plus("india").isNotACelebrityName();
    lets.assume("nares").isNotARestrictedUserName();
    lets.assume("india").isNotARestrictedDomainName();
    lets.assume().identity("[email protected]").isAvailable();
 
    lets.assume("nares").plus("indian").isNotACelebrityName();
    lets.assume("nares").isNotARestrictedUserName();
    lets.assume("indian").isNotARestrictedDomainName();
    lets.assume().identity("[email protected]").isAvailable();
 
    lets.assume("nares").plus("mumbai").isNotACelebrityName();
    lets.assume("nares").isNotARestrictedUserName();
    lets.assume("mumbai").isNotARestrictedDomainName();
    lets.assume().identity("[email protected]").isAvailable();
 
    List<String> generatedIds = suggester.generateIdsFor(naresh_from_mumbai);
 
    lets.assertThat(generatedIds).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

@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("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}

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

1
2
3
4
5
6

@Test
public void avoidRestrictedWordsInIds() {
    lets.assume("naresh").isARestrictedUserName();
    List<String> generatedIds = suggester.suggestIdsFor(naresh_from_mumbai);
    lets.assertThat(generatedIds).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}

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 | No Comments »

Refactoring Teaser II: Solution: Take 1

Tuesday, August 4th, 2009

Following is the first take at refactoring the II Teaser.

Started off by refactoring the tests:

@Test
public void mineSenderFromEdgeServerRecordInMailHeaders() {
    header(
            mail_from("67.205.47.130", "agileindia.org").receivedBy("75.119.213.4", "mails.agileindia.org"),
            mail_from(null, "mail-vw0-f172.google.com").receivedBy("67.205.47.130", "agileindia.org"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(2);
}
@Test
public void useSenderIPForInvalidSenderEdgeServerDomainName() {
    header(
            mail_from("67.205.47.130", "agileindia.org").receivedBy("75.119.213.4", "mails.agileindia.org"),
            mail_from("209.85.212.172", "cannot-exist.agilefaqs.com").receivedBy("67.205.47.130", "agileindia.org"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(2);
}
@Test
public void senderNameCanBeIPAddress() {
    header(mail_from(null, "209.85.212.172").receivedBy("67.205.47.130", "agileindia.org"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(1);
}
@Test
public void matchMXRecordIPWithReciever() {
    header(mail_from("", "mail-vw0-f172.google.com").receivedBy("67.205.47.130", "apache2-echo.robin.dreamhost.com"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(1);
}
@Test
public void skipHeaderRecordsThatDontCrossEdgeServers() {
    header(
            mail_from("192.168.1.47", "smtp.gmail.com").receivedBy("75.119.213.4", "mail.gmail.com"),
            mail_from("192.168.1.3", "192.168.6.242").receivedBy("192.168.1.47", "smtp.gmail.com"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertFalse(senderIP.isValid());
    assertDistanceOfMatchingHeaderFromTopIs(0);
}

Following is the crux of the Sender Edge Server IP Extraction Algo:

public IPAddressExtractor(final List receivedHeaders, final Domain recepientDomain) {
    Domain recepientMXRecord = recepientDomain.retrieveFirstMXRecord();
    for (MatchingCriterion critierion : asList(MATCHING_DOMAIN, MATCHING_IP, MATCHING_SECOND_LEVEL_DOMAIN)) {
        Match result = foreach(receivedHeaders).and(recepientMXRecord).match(critierion);
        if (result.success()) {
            storeSenderIPWithDistance(result);
            break;
        }
    }
}

To do the whole Fluent interfaces on line number 21, I had to create a private method:

private CriterionMatcher foreach(final List mailHeaders) {
    return new CriterionMatcher(mailHeaders);
}

and a package protected CriterionMatcher class

class CriterionMatcher {
    private final List mailHeaders;
    private int counter;
    private Domain mxRecord;
 
    CriterionMatcher(final List mailHeaders) {
        this.mailHeaders = mailHeaders;
    }
 
    CriterionMatcher and(final Domain mxRecord) {
        this.mxRecord = mxRecord;
        return this;
    }
 
    Match match(final MatchingCriterion criterion) {
        for (EmailHeader header : mailHeaders) {
            counter++;
 
            if (criterion.isSatisfiedBy(mxRecord, header)) {
                return new Match(header.fromDomain, header.fromIp, counter);
            }
        }
        return Match.NULL;
    }
}

Other than the switch statement smell and conditional complexity, the original code was obsessed with Primitive Obsession code smell. To fix this issue, the first thing I had to do was great first class citizens (Objects). So I ended up creating

public class IPAddress {
    private static final String IP_ADDRESS_REGEX = "\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b";
    private static final Pattern VALID_IP = Pattern.compile(IP_ADDRESS_REGEX);
    private static final String LOCAL_HOST = "127.0.0.1";
    public static final IPAddress NULL = new IPAddress("");
 
    private final String ip;
 
    private IPAddress(final String address) {
        ip = address;
    }
 
    public static IPAddress parse(final String address) {
        if (address == null) {
            return NULL;
        }
        Matcher matcher = VALID_IP.matcher(address);
 
        if (!matcher.find()) {
            return NULL;
        }
        return new IPAddress(matcher.group(0));
    }
 
    @Override
    public String toString() {
        return ip;
    }
 
    public boolean isLocalhost() {
        return LOCAL_HOST.equals(ip);
    }
 
    public boolean isValid() {
        return this != NULL;
    }
}

and

public class Domain {
    private static final Pattern SLD = Pattern.compile("(.*\\.)?(.*\\..*)");
    public static final Domain NULL = new Domain("", Network.NULL);
 
    private final String name;
    private final Network network;
 
    protected Domain(final String domainName, final Network network) {
        name = domainName.toLowerCase();
        this.network = network;
    }
 
    public IPAddress resolveIP() {
        try {
            String ipAddress = network.ipAddressOf(name);
            return IPAddress.parse(ipAddress);
        } catch (UnknownHostException e) {
            return IPAddress.NULL;
        }
    }
 
    public Domain secondLevelDomain() {
        Matcher mxRecordMatch = SLD.matcher(name);
        if (mxRecordMatch.find()) {
            return new Domain(mxRecordMatch.group(2), network);
        }
        return this;
    }
 
    public Domain retrieveFirstMXRecord() {
        List mxRecords = network.firstMXRecordFor(name);
        if (mxRecords.size() &gt; 0) {
            return new Domain(mxRecords.get(0), network);
        }
        return NULL;
    }
 
    public boolean isValid() {
        return this != NULL;
    }
}

To create a Domain, we use a static Factory called DomainFactory

public final class DomainFactory {
    private static final String DOMAIN_NAME_REGEX = "[\\w.-]+\\.[A-Za-z]{2,6}";
    private static final int MAX_DOMAIN_NAME_LENGTH = 255;
 
    public static Domain build(final String domainName, final Network network) {
        if (isValidDomain(domainName)) {
            return new Domain(domainName, network);
        }
        IPAddress ip = IPAddress.parse(domainName);
        if (ip.isValid()) {
            return retrieveDomainName(ip, network);
        }
        return Domain.NULL;
    }
 
    private static Domain retrieveDomainName(final IPAddress ip, final Network network) {
        try {
            String hostName = network.hostNameFor(ip.toString());
            if (ip.toString().equals(hostName)) {
                return Domain.NULL;
            }
            return new Domain(hostName, network);
        } catch (UnknownHostException e) {
            return Domain.NULL;
        }
    }
}

Finally we’ve the 3 Criterion for checking if a header contains the edge server

public abstract class MatchingCriterion {
    public static final MatchingCriterion MATCHING_DOMAIN = new MatchingDomainCriterion();
    public static final MatchingCriterion MATCHING_IP = new MatchingIPCriterion();
    public static final MatchingCriterion MATCHING_SECOND_LEVEL_DOMAIN = new MatchingSecondLevelDomainCriterion();
 
    public boolean isSatisfiedBy(final Domain mxRecord, final EmailHeader header) {
        return header.fromDomain.isValid() &amp;&amp; satisfies(mxRecord, header);
    }
 
    protected abstract boolean satisfies(Domain mxRecord, EmailHeader header);
}
private static class MatchingDomainCriterion extends MatchingCriterion {
    @Override
    protected boolean satisfies(final Domain mxRecord, final EmailHeader header) {
        return !(header.byIp.equals(header.fromIp) || header.fromIp.isLocalhost() || !header.byDomain.equals(mxRecord));
    }
}
private static class MatchingIPCriterion extends MatchingCriterion {
    @Override
    protected boolean satisfies(final Domain mxRecord, final EmailHeader header) {
        return header.byIp.equals(mxRecord.resolveIP());
    }
}
private static class MatchingSecondLevelDomainCriterion extends MatchingCriterion {
    @Override
    protected boolean satisfies(final Domain mxRecord, final EmailHeader header) {
        Domain secondLevelDomain = mxRecord.secondLevelDomain();
        return secondLevelDomain.equals(header.byDomain.secondLevelDomain());
    }
}

Also notice that for testing purpose we don’t want to hit the network, so I created a FakeNetwork class which stubs out all Network calls. Network is injected into all Domain classes through the DomainFactory. (I’m not very happy with this design, it feels like a bit of a hack to inject Network this way.)

public class FakeNetwork extends Network {
    private static final Map domain2ip = new HashMap() {
        {
            put("mail-vw0-f172.google.com", "209.85.212.172");
            put("209.85.212.172", "mail-vw0-f172.google.com");
            put("mails.agileindia.org", "72.14.203.121");
            put("agileindia.org", "67.205.47.130");
        }
    };
 
    @Override
    public String ipAddressOf(final String domainName) throws UnknownHostException {
        return lookup(domainName);
    }
 
    @Override
    public String hostNameFor(final String ipAddress) throws UnknownHostException {
        return lookup(ipAddress);
    }
 
    @Override
    public List firstMXRecordFor(final String name) {
        return asList("agileindia.org");
    }
 
    private String lookup(final String value) throws UnknownHostException {
        String data = domain2ip.get(value);
        if (data == null) {
            throw new UnknownHostException();
        }
        return data;
    }
}

Feel free to download the whole project.

Posted in Agile, Design, Programming, Testing | 2 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;
1
2
3
4
5
6
7
8
9
10

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();
1
2
3
4

public void AssertThat(ThingsToBeVerified codeBlock)
{
  codeBlock();
}
1
2
3
4
5
6
7
8
9
10
11
12
13
14

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)

1
2
3
4
5
6
7
8
9
10
11

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 | No Comments »

Fluent Interfaces improve readability of my Tests

Saturday, December 27th, 2008

Recently I was working on a Domain Forwarding Server. For Ex. if you request for http://google.com and Google wants to redirect you to http://google.in, then this server handles the redirection for you. There are many permutations and combination of how you want to forward your domain to another domain.

First pass, I wrote a tests:

1
2
3
4
5
6
7
8

@Test
public void permanentlyRedirectDomainsWithPath()  {
    when(request.hostName()).thenReturn("google.com");
    when(request.protocol()).thenReturn("HTTP/1.1");
    when(request.path()).thenReturn("/index.html");
    domain.setDomain("google.com");
    domain.forwardPath(true);
    domain.setForward("google.in");
9

    response = service.processMessage(request);
10
11
12
13

    assertStatus(StatusCode.PermanentRedirect);
    assertLocation("google.in/index.html");
    assertStandardResponseHeader();
}

Note that since the request is an expensive object to build, I’m stubbing it out. While domain is more of a value object in this case, so its not stubbed/mocked out.

This test has a lot of noise. All the stubbing logic was just making this test very difficult to understand what was going on. So I extracted some methods to make it more meaningful and hide the stubbing logic.

1
2
3
4
5
6
7

@Test
public void permanentlyRedirectDomainsWithPath()  {
    setDomainToBeRedirected("google.com");
    setDestinationDomain("google.in");
    shouldRedirectWithPath();
    setHostName("google.com");
    setPath("/index.html");
8

    response = service.processMessage(request);
9
10
11
12

    assertStatus(StatusCode.PermanentRedirect);
    assertLocation("google.in/index.html");
    assertStandardResponseHeader();
}

Looking at this code it occurred to me that I could use Fluent Interfaces and make this really read like natural language. So finally I got:

1
2
3
4

@Test
public void permanentlyRedirectDomainsWithPath()  {
    request("google.com").withPath("/index.html");
    redirect("google.com").withPath().to("google.in");
5

    response = service.processMessage(request);
6
7
8
9

    assertStatus(StatusCode.PermanentRedirect);
    assertLocation("google.in/index.html");
    assertStandardResponseHeader();
}
10
11
12
13
14

private ThisTest request(String domainName) {
    when(request.hostName()).thenReturn(domainName);
    when(request.protocol()).thenReturn("HTTP/1.1");
    return this;
}
15
16
17

private void withPath(String path) {
    when(request.path()).thenReturn(path);
}
18
19
20
21

private ThisTest redirect(String domainName) {
    domain.setDomain(domainName);
    return this;
}
22
23
24
25

private ThisTest withPath() {
    domain.forwardPath(true);
    return this;
}
26
27
28

private void to(String domainName) {
    domain.setForward(domainName);
}

Finally after introducing Context Objects, I was able to make the code even more easier to read and understand:

1
2
3
4
5
6
7
8
9

@Test
public void redirectSubDomainsPermanently() {
    lets.assume("google.com").getsRedirectedTo("google.in").withPath();
    response = domainForwardingServer.process(requestFor("google.com/index.html"));
    lets.assertThat(response).contains(StatusCode.PermanentRedirect)
                             .location("google.in/index.html").protocol("HTTP/1.1")
                             .connectionStatus("close").contentType("text/html")
                             .serverName("Directi Server 2.0");
}

Posted in Agile, Testing | 2 Comments »
    Licensed under
Creative Commons License