XNSIO
  About   Slides   Home  

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

Confronting the Fear of Legacy Code

Wednesday, September 16th, 2009

When faced with Legacy Code, I’ve found 3 possible options to deal with them:

  • Leave it alone for now: Very rarely used, code seems to work fine.
  • Piecemeal Refactoring: When its difficult to understand what the code does and how it does what it does. Its time for safe, slow and cumbersome refactoring process.
  • Rewrite: When its clear what the code does, but it very difficult to understand how it does what it does, it time to rescue the code by rewriting it from scratch. This can be applied at various levels (whole code base, single module, class or method).

To Rewrite or to Refactor?

One can easily spend hours or days trying to refactor some code, when clearly (in retrospect) rewriting the code would be a better option. Sometimes you decide its better to rewrite the code and end up implementing something that does not work in all situations or we miss out something important. Unfortunately there is no clear guideline when I would choose to refactor code v/s rewrite the code. The key to me is, if I understand what the code does not necessarily how it does what it does, then its time to rewrite the code.

Rewriting code: Play it safe

The analogy I use is, rewriting code is like building bridges. You know that the bridge helps you get from point A to point B. It might be very complicated and risky to use the bridge any more. But that does not mean you’ll go and blow the bridge apart. Instead you would slowing start building a new bridge along side. When the new bridge is ready, you would divert a sample traffic on this bridge and see if it actually works. If it does, then you migrate all the traffic to the new bridge and blow the old bridge apart.

I use the very same technique when rewriting code. During the process, I might leave the code working but in a much more messier (worse) state. During CodeChef TechTalks in Bangalore, Sai told me that he refers to this as an “Expand and Contract” cycle. You are temporarily expanding your code base so that you can come back and clean it up.

When I’m rewriting code, I find black-box style automated tests very helpful. If you don’t have tests, it might be worth investing the time to write a few.

Where to begin Refactoring Code

  • Outside-In: Start from a higher-level and refactor (delve) into the crux
  • Inside-Out: Start refactoring the crux and work your way out

At times its difficult to identify the crux and I spend some time exploring (via refactoring) before I can choose an approach. Tests can be a great probe to understand the code.

When refactoring legacy code, I usually use the Scaffolding Technique to break the Catch 22 situation (To refactor we need tests, to write tests we need to refactor). Scaffolding tests don’t necessarily have to be UI tests, I’ve used Unit tests as scaffolding tests as well.The key thing is they are temporary and meant to help you get started.

Thanks to the folks @ the Legacy Code BoF @ CodeChef TechTalks in Bangalore who prompted me to write this blog.

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 && 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 && 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 IV – Step 1

Wednesday, August 12th, 2009

So far, most of the refactoring teasers we’ve looked at, have suffered because of lack of modularity and with primitive obsession. This refactoring teaser is quite the opposite. Overall the code base is decent sized. So instead of trying to solve the whole problem in one go, let’s take it one step at a time.

Download the Source Code here: Java or C#.

In the first step, I want you to focus on the PartialAcceptanceTest.

Test Setup:

private final Country country = new Country("IN", "India", "Indian");
private final LocationInformation location = new LocationInformation(country, "Mumbai");
private final UserService userService = createMock(UserService.class);
private final DomainNameService domainNameService = createMock(DomainNameService.class);
private final SuggesntionsUtil utils = new SuggesntionsUtil(domainNameService);
private final RandomNumberGenerator randomNumberGenerator = new RandomNumberGenerator() {
    @Override
    public String next() {
        return "_random";
    }
};
private final ChildSuggestionFactory childSuggestionFactory = new ChildSuggestionFactory(userService, utils, randomNumberGenerator);
private final SuggestionStrategyFactory suggestionsFactory = new SuggestionStrategyFactory(childSuggestionFactory);
private final IdentityGenerator identityGenerator = new IdentityGenerator(suggestionsFactory);

First Test (Happy Path)

@Test
public void generateIdsUsingNameLocationAndNationality() {
    expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("[email protected]", "[email protected]", "[email protected]", "[email protected]");
 
    assertEquals(expectedIds, generatedIDs);
 
    verify(userService, domainNameService);
}

Second Test

@Test
public void avoidRestrictedWordsInIds() {
    expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn("Naresh");
 
    expect(domainNameService.isCelebrityName("Nares", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Nares", "India")).andStubReturn(false);
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Nares", "Indian")).andStubReturn(false);
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Nares", "Mumbai")).andStubReturn(false);
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("[email protected]", "[email protected]", "[email protected]", "[email protected]");
 
    assertEquals(expectedIds, generatedIDs);
 
    verify(userService, domainNameService);
}
@Test
public void avoidCelebrityNamesInGeneratedIds() {
    expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Nares", "Jai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("[email protected]", "[email protected]", "[email protected]", "[email protected]");
 
    assertEquals(expectedIds, generatedIDs);
 
    verify(userService, domainNameService);
}
@Test
public void appendCurrentYearWithFirstNameIfIdIsNotAvailable() {
    expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(false);
 
    expect(domainNameService.isCelebrityName("Naresh2009", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh2009")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("[email protected]", "[email protected]", "[email protected]", "[email protected]");
 
    assertEquals(expectedIds, generatedIDs);
 
    verify(userService, domainNameService);
}
@Test
public void appendRandomNumberWithFirstNameIfIdIsNotAvailable() {
    expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(false);
 
    expect(domainNameService.isCelebrityName("Naresh2009", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh2009")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(false);
 
    expect(domainNameService.isCelebrityName("Naresh_random", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh_random")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("[email protected]")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("[email protected]", "[email protected]", "[email protected]", "[email protected]");
 
    assertEquals(expectedIds, generatedIDs);
 
    verify(userService, domainNameService);
}

Some helper method:

private List ids(final String... ids) {
    return Arrays.asList(ids);
}

Download the Source Code here: Java or C#.

Refactoring Teaser III

Wednesday, August 5th, 2009

This time a simple one.

Following test will help you understand the code:

public class MyLoggerTest implements Console {
    private String msg;
    private MyLogger logger = new MyLogger(this);
 
    @Test
    public void handleNonIOExceptions() {
        logger.error(new IllegalArgumentException("Ignore Exception"));
        assertEquals("SEVERE: Dying due to exception : Ignore Exception", msg);
    }
 
    @Test
    public void ignoreSpecificIOExceptions() {
        String errorMsg = "Broken pipe:" + Math.random();
        logger.error(new IOException(errorMsg));
        assertEquals("FINE: Ignoring Exception for : " + errorMsg, msg);
    }
 
    @Test
    public void handleGenericIOExceptions() {
        String errorMsg = "Random IO Error:" + Math.random();
        logger.error(new IOException(errorMsg));
        assertEquals("SEVERE: Dying due to exception : " + errorMsg, msg);
    }
 
    @Override
    public void write(final String msg) {
        this.msg = msg;
    }
}
public class MyLogger {
    private final Console out;
 
    public MyLogger(final Console out) {
        this.out = out;
    }
 
    private static final String[] IGNORED_IOEXCEPTION_MESSAGES = {
            "An existing connection was forcibly closed by the remote host",
            "Connection reset by peer",
            "Broken pipe",
            "Connection timed out",
            "No route to host",
            };
 
    public void error(final Throwable t) {
        if (isIgnored(t)) {
            out.write("FINE: Ignoring Exception for : " + t.getMessage());
        } else {
            out.write("SEVERE: Dying due to exception : " + t.getMessage());
        }
    }
 
    private boolean isIgnored(final Throwable t) {
        if (t instanceof IOException) {
            final String exceptionMessage = t.getMessage();
            for (String ignoredMessage : IGNORED_IOEXCEPTION_MESSAGES) {
                if (exceptionMessage.startsWith(ignoredMessage)) {
                    return true;
                }
            }
        }
        return false;
    }
}

and

public interface Console {
    void write(String msg);
}

Feel free to download the Java project.

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() > 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() && 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.

Refactoring Teaser 2

Monday, July 20th, 2009

Here comes the second Refactoring Teaser.

The purpose of this code is to determine the sender domain and IP address of the email server used by the sender of an email.

Following tests explain the purpose in more detail:

public class IPAddressExtractorTest {
    private ArrayList header = new ArrayList();
 
    @Test
    public void mineSenderFromEdgeServerRecordInMailHeaders() {
        mail_from("67.205.47.130", "agileindia.org").RecievedBy("75.119.213.4", "mails.agileindia.org");
        mail_from(null, "mail-vw0-f172.google.com").RecievedBy("67.205.47.130", "agileindia.org");
        assertSenderIPIs("209.85.212.172");
    }
 
    @Test
    public void useSenderIPForInvalidSenderEdgeServerDomainName() {
        mail_from("67.205.47.130", "agileindia.org").RecievedBy("75.119.213.4", "mails.agileindia.org");
        mail_from("209.85.212.172", "cannot-exist.agilefaqs.com").RecievedBy("67.205.47.130", "agileindia.org");
        assertSenderIPIs("209.85.212.172");
    }
 
    @Test
    public void senderNameCanBeIPAddress() {
        mail_from(null, "209.85.212.172").RecievedBy("67.205.47.130", "agileindia.org");
        assertSenderIPIs("209.85.212.172");
    }
 
    @Test
    public void matchMXRecordIPWithReciever() {
        mail_from("", "mail-vw0-f172.google.com").RecievedBy("67.205.47.130", "apache2-echo.robin.dreamhost.com");
        assertSenderIPIs("209.85.212.172");
    }
 
    @Test
    public void skipHeaderRecordsThatDontCrossEdgeServers() {
        mail_from("192.168.1.47", "smtp.gmail.com").RecievedBy("75.119.213.4", "mail.gmail.com");
        mail_from("192.168.1.3", "192.168.6.242").RecievedBy("192.168.1.47", "smtp.gmail.com");
        assertSenderIPIs("");
 
    }
 
    private void RecievedBy(final String ip, final String domainName) {
        header.add(new ReceiveFromByHeaders(ip, fromIp, domainName, fromDomain));
    }
 
    private String fromIp;
    private String fromDomain;
 
    private IPAddressExtractorTest mail_from(final String ip, final String domainName) {
        fromIp = ip;
        fromDomain = domainName;
        return this;
    }
 
    private void assertSenderIPIs(final String senderIP) {
        IPAddressExtractor addressExtractor = new IPAddressExtractor(header, "gmail.com");
        assertEquals(senderIP, addressExtractor.getSenderIP());
    }
}

Following is our big ball of mud:

public class IPAddressExtractor {
    private static Pattern regexSLD = Pattern.compile("(.*\\.)?(.*\\..*)");
    private static Pattern regexIP = Pattern.compile(Constants.ValidIpAddressRegex);
    private static Logger logger = Logger.getLogger(IPAddressExtractor.class.getName());
 
    private List _receiveFromByHeaders;
    private String _recepientDomain;
 
    private String _senderIP;
    private int _distance;
 
    public IPAddressExtractor(final List receiveFromByHeaders, final String recepientDomain) {
        logger.info("Entering IPAddressExtractor");
        logger.info(recepientDomain);
 
        _receiveFromByHeaders = receiveFromByHeaders;
        _recepientDomain = recepientDomain;
 
        ExtractSenderIPfromReceiveHeadersFromTop();
 
        logger.info("Leaving IPAddressExtractor");
    }
 
    private void ExtractSenderIPfromReceiveHeadersFromTop() {
        String senderDomain = "";
        boolean gotSenderDomain = false;
 
        String[] mxRecords = null;
 
        try {
            mxRecords = DnsMx.GetMXRecords(_recepientDomain);
 
            if (mxRecords == null) {
                return;
            }
        } catch (Exception ex) {
            // no records found
            return;
        }
        // generally first MX record is considered;
        String mxRecordIP = ResolveIPAddress(mxRecords[0]);
        logger.info(mxRecords[0] + " " + mxRecordIP);
 
        String ipByRecipMXServer = "";
 
        // exact MX Match
        int counter = 0;
        for (ReceiveFromByHeaders rOBj : _receiveFromByHeaders) {
            counter++;
 
            if (rOBj.getReceiveByIpAddress() != null
                    && (rOBj.getReceiveByIpAddress().equals(rOBj.getReceiveFromIpAddress()) || "127.0.0.1".equals(rOBj
                            .getReceiveFromIpAddress()))) {
                continue;
            }
 
            if (mxRecords[0].toLowerCase() == rOBj.getReceiveByHeader().toLowerCase()) {
                if (VerifyDomain(rOBj.getReceiveFromHeader()) && !VerifyIPAddress(rOBj.getReceiveFromHeader())) {
                    senderDomain = rOBj.getReceiveFromHeader();
                    gotSenderDomain = true;
                    ipByRecipMXServer = rOBj.getReceiveFromIpAddress();
                    _distance = counter;
                    break;
                } else if (VerifyIPAddress(rOBj.getReceiveFromHeader()))// since somethimes theres an ipAddress instead
                // of domain
                {
                    senderDomain = GetHostName(rOBj.getReceiveFromHeader());
                    gotSenderDomain = true;
                    _distance = counter;
                    break;
                }
            }
        }
 
        // MX IP match
        if (!gotSenderDomain) {
            counter = 0;
 
            for (ReceiveFromByHeaders rOBj : _receiveFromByHeaders) {
                counter++;
                if (mxRecordIP.equals(rOBj.getReceiveByIpAddress())) {
                    if (VerifyDomain(rOBj.getReceiveFromHeader()) && !VerifyIPAddress(rOBj.getReceiveFromHeader())) {
                        senderDomain = rOBj.getReceiveFromHeader();
                        gotSenderDomain = true;
                        ipByRecipMXServer = rOBj.getReceiveFromIpAddress();
                        _distance = counter;
                        break;
                    } else if (VerifyIPAddress(rOBj.getReceiveFromHeader()))// since somethimes theres an ipAddress
                    // instead of domain
                    {
                        senderDomain = GetHostName(rOBj.getReceiveFromHeader());
                        gotSenderDomain = true;
                        _distance = counter;
                        break;
                    }
                }
            }
        }
 
        // MX SLD match
        if (!gotSenderDomain) {
            counter = 0;
 
            for (ReceiveFromByHeaders rOBj : _receiveFromByHeaders) {
                counter++;
 
                Matcher mxRecordMatch = regexSLD.matcher(mxRecords[0]);
                Matcher rOBJMatch = regexSLD.matcher(rOBj.getReceiveByHeader());
 
                if (!(mxRecordMatch.find() && rOBJMatch.find())) {
                    continue;
                }
 
                if (mxRecordMatch.group(2).toLowerCase() == rOBJMatch.group(2).toLowerCase()) {
                    if (VerifyDomain(rOBj.getReceiveFromHeader()) && !VerifyIPAddress(rOBj.getReceiveFromHeader())) {
                        senderDomain = rOBj.getReceiveFromHeader();
                        gotSenderDomain = true;
                        ipByRecipMXServer = rOBj.getReceiveFromIpAddress();
                        _distance = counter;
                        break;
                    } else if (VerifyIPAddress(rOBj.getReceiveFromHeader()))// since somethimes theres an ipAddress
                    // instead of domain
                    {
                        String extractIP = ExtractIP(rOBj.getReceiveFromHeader());
                        senderDomain = GetHostName(extractIP);
                        gotSenderDomain = true;
                        _distance = counter;
                        break;
                    }
                }
            }
        }
 
        String ipAddress = "";
 
        try {
            if (senderDomain != null && senderDomain.trim().length() > 0) {
                ipAddress = ResolveIPAddress(senderDomain);
            }
        } catch (Exception e) {
        }
 
        if (ipAddress == null || ipAddress.trim().length() == 0) {
            ipAddress = ipByRecipMXServer;
        }
 
        _senderIP = ipAddress;
    }
 
    // sometimes IP can enclosed in brackets or extra chars
    private String ExtractIP(final String str) {
        logger.info("Entering ExtractIP");
        logger.info(str);
 
        return regexIP.matcher(str).group(1);
    }
 
    private String ResolveIPAddress(final String domain) {
        String ipAddress = "";
 
        if (!(domain.length() == 0 || domain.length() > Constants.MaxDomainLength || !Pattern.matches(Constants.DomainNameRegex, domain))) {
            try {
                ipAddress = InetAddress.getByName(domain).getHostAddress();
            } catch (UnknownHostException e) {
                logger.log(Level.INFO, "Not a valid Domain Name " + domain);
            }
            logger.info("IPAddress " + ipAddress + " found for domain " + domain);
        } else {
            logger.log(Level.INFO, "Not a valid Domain Name " + domain);
        }
        return ipAddress;
    }
 
    private boolean VerifyDomain(final String senderDomain) {
 
        if (senderDomain != null && senderDomain.trim().length() > 0) {
            if (!(senderDomain.length() == 0 || senderDomain.length() > Constants.MaxDomainLength || !Pattern.matches(
                    Constants.DomainNameRegex, senderDomain))) {
                logger.log(Level.FINE, "Sender domain identified as " + senderDomain);
                return true;
            } else {
                logger.log(Level.FINE, "Sender domain identified is not a valid Domain Name " + senderDomain);
                return false;
            }
        }
 
        return true;
    }
 
    private boolean VerifyIPAddress(final String ipAddress) {
        logger.info("Entering VerifyAddress");
        logger.info(ipAddress);
 
        if (ipAddress != null && ipAddress.trim().length() > 0) {
            return regexIP.matcher(ipAddress).find();
        }
 
        return false;
    }
 
    private String GetHostName(final String ipAddress) {
        try {
            InetAddress ip = InetAddress.getByName(ipAddress);
            return ip.getHostName();
        } catch (UnknownHostException e) {
            e.printStackTrace();
        }
        return "";
    }
 
    public String getSenderIP() {
        return _senderIP;
    }
 
    public int getDistance() {
        return _distance;
    }
}

This class depends on:

public class ReceiveFromByHeaders {
    private String receiveByIpAddress;
    private String receiveFromIpAddress;
    private String receiveByHeader;
    private String receiveFromHeader;
 
    public ReceiveFromByHeaders(final String receiveByIpAddress, final String receiveFromIpAddress, final String receiveByHeader,
            final String receiveFromHeader) {
        this.receiveByIpAddress = receiveByIpAddress;
        this.receiveFromIpAddress = receiveFromIpAddress;
        this.receiveByHeader = receiveByHeader;
        this.receiveFromHeader = receiveFromHeader;
    }
 
    public String getReceiveByIpAddress() {
        return receiveByIpAddress;
    }
 
    public void setReceiveByIpAddress(final String receiveByIpAddress) {
        this.receiveByIpAddress = receiveByIpAddress;
    }
 
    public String getReceiveFromIpAddress() {
        return receiveFromIpAddress;
    }
 
    public void setReceiveFromIpAddress(final String receiveFromIpAddress) {
        this.receiveFromIpAddress = receiveFromIpAddress;
    }
 
    public String getReceiveByHeader() {
        return receiveByHeader;
    }
 
    public void setReceiveByHeader(final String receiveByHeader) {
        this.receiveByHeader = receiveByHeader;
    }
 
    public String getReceiveFromHeader() {
        return receiveFromHeader;
    }
 
    public void setReceiveFromHeader(final String receiveFromHeader) {
        this.receiveFromHeader = receiveFromHeader;
    }
}

Some Contants:

public class Constants {
    public static final String ValidIpAddressRegex = "\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b";
    public static final int MaxDomainLength = 255;
    public static final String DomainNameRegex = "[\\w.-]+\\.[\\w-[0123456789]]{2,6}";
}

Finally, we’ve used the following class to stub out MxRecord look up:

public class DnsMx {
    public static String[] GetMXRecords(final String domain) {
        return new String[] { "agileindia.org", "alt2.gmail-smtp-in.l.google.com" };
    }
}

Download the Java Project or C# Version.

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 > 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 > 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 < words.length; ++i) {
            phrases.add(words[i]);
        }
    }
 
    private void buildDoubleWordPhrases(final String[] words) {
        for (int i = 0; i < words.length - 1; ++i) {
            phrases.add(words[i] + SPACE + words[i + 1]);
        }
    }
 
    private void buildTripleWordPhrases(final String[] words) {
        for (int i = 0; i < 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 < words.length; ++i) {
            phrases.add(words[i]);
        }
    }
 
    private void buildDoubleWordPhrases(final String[] words) {
        for (int i = 0; i < words.length - 1; ++i) {
            phrases.add(words[i] + SPACE + words[i + 1]);
        }
    }
 
    private void buildTripleWordPhrases(final String[] words) {
        for (int i = 0; i < 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 <= 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 < 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 > minNoWords;
        }
 
        List phrasesOf(final int length) {
            List phrases = new ArrayList();
            for (int i = 0; i <= 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 < 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.

Refactoring Teaser Part 1

Wednesday, July 8th, 2009

How would you refactoring the following code? (This code is in Java, but you can refactoring using any language of your choice).

Following test explains the functionality of the production code:

1
2
3
4
5
6
7
8
9
public class StringUtilTest {
  @Test
  public void testSplit() {
    assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", StringUtil.split("Hello World Java", 6));
    assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", StringUtil.split("Hello World Java", 10));
    assertEquals("'Hello', 'World', 'Java', 'Hello World'", StringUtil.split("Hello World Java", 4));
    assertEquals("'Hello'", StringUtil.split("Hello World Java", 1));
  }
}

General use case is that for a given string (content), users might want split the same string to get different numbers of keywords in the output.

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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
public final class StringUtil {
  private static final Pattern REGEX_TO_SPLIT_ALONG_WHITESPACES = Pattern.compile("\\p{Z}|\\p{P}");
 
  public static String split(final String content, final int number) {
    String listOfKeywords = "";
    int count = 0;
    String[] tokens = REGEX_TO_SPLIT_ALONG_WHITESPACES.split(content);
    List<string> strings = Arrays.asList(tokens);
    List<string> allStrings = singleDoubleTripleWords(strings);
    int size = allStrings.size();
    for (String phrase : allStrings) {
      if (count == number) {
        break;
      }
      listOfKeywords += "'" + phrase + "'";
      if (++count < size && count < number) {
        listOfKeywords += ", ";
      }
    }
    return listOfKeywords;
  }
 
  private static List<String> singleDoubleTripleWords(final List<string> strings) {
    List<string> allStrings = new ArrayList<string>();
    int numWords = strings.size();
 
    if (hasEnoughWords(numWords) == false) {
      return allStrings;
    }
 
    // Extracting single words. Total size of words == numWords
 
    // Extracting single-word phrases.
    for (int i = 0; i < numWords; ++i) {
      allStrings.add(strings.get(i));
    }
 
    // Extracting double-word phrases
    for (int i = 0; i < numWords - 1; ++i) {
      allStrings.add(strings.get(i) + " " + strings.get(i + 1));
    }
 
    // Extracting triple-word phrases
    for (int i = 0; i < numWords - 2; ++i) {
      allStrings.add(strings.get(i) + " " + strings.get(i + 1) + " " + strings.get(i + 2));
    }
    return allStrings;
  }
 
  private static boolean hasEnoughWords(final int numWords) {
    if (numWords < 3) {
      return false;
    }
    return true;
  }
}

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);
}

Another Project Rescue Report

Monday, February 9th, 2009

Some time back, I spent 1 Week helping a project (Server written in Java) clear its Technical Debt. The code base is tiny because it leverages lot of existing server framework to do its job. This server handles extremely high volumes of data & request and is a very important part of our server infrastructure. Here are some results:

Topic Before After
Project Size Production Code

  • Package =1
  • Classes =4
  • Methods = 15 (average 3.75/class)
  • LOC = 172 (average 11.47/method and 43/class)
  • Average Cyclomatic Complexity/Method = 3.27

Test Code

  • Package =0
  • Classes = 0
  • Methods = 0
  • LOC = 0
Production Code

  • Package = 4
  • Classes =13
  • Methods = 68 (average 5.23/class)
  • LOC = 394 (average 5.79/method and 30.31/class)
  • Average Cyclomatic Complexity/Method = 1.58

Test Code

  • Package = 6
  • Classes = 11
  • Methods = 90
  • LOC =458
Code Coverage
  • Line Coverage: 0%
  • Block Coverage: 0%

Old Code Coverage Report

  • Line Coverage: 96%
  • Block Coverage: 97%

New Code Coverage Report

Cyclomatic Complexity Cyclomatic Complexity report before Refactoring Cyclomatic Complexity report after Refactoring
Obvious Dead Code Following public methods:

  • class DatabaseLayer: releasePool()

Total: 1 method in 1 class

Following public methods:

  • class DFService: overloaded constructor

Total: 1 method in 1 class

Note: This method is required by the tests.

Automation
Version Control Usage
  • Average Commits Per Day = 0
  • Average # of Files Changed Per Commit = 12
  • Average Commits Per Day = 7
  • Average # of Files Changed Per Commit = 4
Coding Convention Violation 96 0

Another similar report.

    Licensed under
Creative Commons License