Naresh Jain » Agile FAQs Blog » Managed Chaos

Simplifying a Complex Design

Thursday, July 15th, 2010

From

Posted in Design, Programming | No Comments »

This method is inside a LightWeightXMLParser. It reads data from a socket channel (java nio) and collects data until data is available on the channel. When a message is complete (fully formed XML), you can retrieve messages by invoking the getMsgs() method and you can invoke areThereMsgs() method to know if at least a message is presents.

/*
 * @return an array with all messages found
 */
public String[] getMsgs() {
    String[] res = new String[msgs.size()];
    for (int i = 0; i < res.length; i++)
        res[i] = msgs.get(i);
    msgs.clear();
    invalidateBuffer();
    return res;
}

Following Tests might help you understand the code slightly better:

    @Override
    protected void setUp() throws Exception {
        super.setUp();
        // Create parser
        parser = new LightWeightXMLParser(CHARSET);
        // Crete byte buffer and append text
        in = ByteBuffer.allocate(4096);
    }

    @Override
    protected void tearDown() throws Exception {
        super.tearDown();
        // Release byte buffer
        in.clear();
    }

    public void testHeader() throws Exception {
        String msg1 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        in.put(msg1.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("Stream header is not being correctly parsed", parser.areThereMsgs());
        assertEquals("Wrong stanza was parsed", msg1, parser.getMsgs()[0]);
    }

    public void testHeaderWithXMLVersion() throws Exception {
        String msg1 = "<?xml version=\"1.0\"?>";
        String msg2 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        in.put((msg1 + msg2).getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("Stream header is not being correctly parsed", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 2, values.length);
        assertEquals("Wrong stanza was parsed", msg1, values[0]);
        assertEquals("Wrong stanza was parsed", msg2, values[1]);
    }

    public void testStanzas() throws Exception {
        String msg1 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        String msg2 = "<starttls xmlns=\"urn:ietf:params:xml:ns:xmpp-tls\"/>";
        String msg3 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        String msg4 = "<iq id=\"428qP-0\" to=\"localhost\" type=\"get\"><query xmlns=\"jabber:iq:register\"></query></iq>";
        String msg5 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        String msg6 = "<presence id=\"428qP-5\"></presence>";
        in.put(msg1.getBytes());
        in.put(msg2.getBytes());
        in.put(msg3.getBytes());
        in.put(msg4.getBytes());
        in.put(msg5.getBytes());
        in.put(msg6.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("Stream header is not being correctly parsed", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 6, values.length);
        assertEquals("Wrong stanza was parsed", msg1, values[0]);
        assertEquals("Wrong stanza was parsed", msg2, values[1]);
        assertEquals("Wrong stanza was parsed", msg3, values[2]);
        assertEquals("Wrong stanza was parsed", msg4, values[3]);
        assertEquals("Wrong stanza was parsed", msg5, values[4]);
        assertEquals("Wrong stanza was parsed", msg6, values[5]);
    }

    public void testCompleteStanzas() throws Exception {
        String msg1 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        String msg2 = "<starttls xmlns=\"urn:ietf:params:xml:ns:xmpp-tls\"/>";
        String msg3 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        String msg4 = "<iq id=\"428qP-0\" to=\"localhost\" type=\"get\"><query xmlns=\"jabber:iq:register\"></query></iq>";
        String msg5 = "<stream:stream to=\"localhost\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">";
        String msg6 = "<presence id=\"428qP-5\"></presence>";
        String msg7 = "</stream:stream>";
        in.put(msg1.getBytes());
        in.put(msg2.getBytes());
        in.put(msg3.getBytes());
        in.put(msg4.getBytes());
        in.put(msg5.getBytes());
        in.put(msg6.getBytes());
        in.put(msg7.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("Stream header is not being correctly parsed", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 7, values.length);
        assertEquals("Wrong stanza was parsed", msg1, values[0]);
        assertEquals("Wrong stanza was parsed", msg2, values[1]);
        assertEquals("Wrong stanza was parsed", msg3, values[2]);
        assertEquals("Wrong stanza was parsed", msg4, values[3]);
        assertEquals("Wrong stanza was parsed", msg5, values[4]);
        assertEquals("Wrong stanza was parsed", msg6, values[5]);
        assertEquals("Wrong stanza was parsed", msg7, values[6]);
    }

    public void testIQ() throws Exception {
        String iq = "<iq type=\"set\" to=\"lachesis\" from=\"0sups/Connection Worker - 1\" id=\"360-22348\"><session xmlns=\"http://jabber.org/protocol/connectionmanager\" id=\"0sups87b1694\"><close/></session></iq>";
        in.put(iq.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("Stream header is not being correctly parsed", parser.areThereMsgs());
        String parsedIQ = parser.getMsgs()[0];
        assertEquals("Wrong stanza was parsed", iq, parsedIQ);
    }

    public void testNestedElements() throws Exception {
        String msg1 = "<message><message xmlns=\"e\">1</message></message>";
        in.put(msg1.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("Stream header is not being correctly parsed", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 1, values.length);
        assertEquals("Wrong stanza was parsed", msg1, values[0]);
    }

    public void testIncompleteStanza() throws Exception {
        String msg1 = "<message><something xmlns=\"http://idetalk.com/namespace\">12";
        in.put(msg1.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertFalse("Found messages in incomplete stanza", parser.areThereMsgs());
    }

    public void testStanzaWithSpecialChars() throws Exception {
        String msg1 = "<message><something xmlns=\"http://idetalk.com/namespace\">12/</something></message>";
        String msg2 = "<message><something xmlns=\"http://idetalk.com/namespace\">12///</something></message>";
        String msg3 = "<message><something xmlns=\"http://idetalk.com/namespace\">12/\\/</something></message>";
        String msg4 = "<message><something xmlns=\"http://idetalk.com/namespace\">http://idetalk.com/namespace/</something></message>";
        in.put(msg1.getBytes());
        in.put(msg2.getBytes());
        in.put(msg3.getBytes());
        in.put(msg4.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("No messages were found in stanza", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 4, values.length);
        assertEquals("Wrong stanza was parsed", msg1, values[0]);
        assertEquals("Wrong stanza was parsed", msg2, values[1]);
        assertEquals("Wrong stanza was parsed", msg3, values[2]);
        assertEquals("Wrong stanza was parsed", msg4, values[3]);
    }

    public void testCompletedStanza() throws Exception {
        String msg1 = "<message><something xmlns=\"http://idetalk.com/namespace\">12";
        in.put(msg1.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertFalse("Found messages in incomplete stanza", parser.areThereMsgs());
 
        String msg2 = "</something></message>";
        ByteBuffer in2 = ByteBuffer.allocate(4096);
        in2.put(msg2.getBytes());
        in2.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in2);
        in2.clear();
        assertTrue("Stream header is not being correctly parsed", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 1, values.length);
        assertEquals("Wrong stanza was parsed", msg1 + msg2, values[0]);
    }

    public void testStanzaWithComments() throws Exception {
        String msg1 = "<iq from=\"lg@jabber.org/spark\"><query xmlns=\"jabber:iq:privacy\"><!-- silly comment --></query></iq>";
        in.put(msg1.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("No messages were found in stanza", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 1, values.length);
        assertEquals("Wrong stanza was parsed", msg1, values[0]);
    }

    public void testWeirdoContent() throws Exception {
        final String[] testStanzas = { "<?xml version=\"1.0\"?>",
                "<stream:stream xmlns:stream=\"http://etherx.jabber.org/streams\" xmlns=\"jabber:client\" to=\"localhost\" >",
                "<emppartag test=\"1\"/>", "<cdatatest><![CDATA[just<ignore everything& >>here<<<<< /> />]]&gt;]]></cdatatest>",
                "<esctest param=\"1\"> this \" is / a test /> test /> </esctest>",
                "<comtest>this <!-- comment --> is a comment</comtest>", "<emptag/>",
                "<iq type=\"get\" id=\"aab1a\" ><query xmlns=\"jabber:iq:roster\"/> <tag> text </tag></iq>",
                "<iq type=\"get\" id=\"aab1a\" ><query xmlns=\"jabber:iq:roster\"/> </iq>",
                "<message><body xmlns=\"http://idetalk.com/namespace\">12\"</body></message>",
                "<message to=\"lg@jabber.org\" id=\"XRk8p-X\"><body> /> /> </body></message>", };
        String testMsg = "";
        for (String s : testStanzas)
            testMsg += s;
        ByteBuffer mybuffer = ByteBuffer.wrap(testMsg.getBytes());
        parser.read(mybuffer);
 
        String[] msgs = parser.getMsgs();
        for (int i = 0; i < testStanzas.length; i++) {
            assertTrue(i < msgs.length);
            assertEquals(testStanzas[i], msgs[i]);
        }
    }

    public void testRead() {
        try {
            LightWeightXMLParser parser = new LightWeightXMLParser("UTF-8");
            String xml1 = "<ab>\u1000</a";
            String xml2 = "b>";
            ByteBuffer buffer1 = ByteBuffer.wrap(xml1.getBytes("UTF-8"));
            ByteBuffer buffer2 = ByteBuffer.wrap(xml2.getBytes("UTF-8"));
 
            parser.read(buffer1);
            parser.read(buffer2);
 
            if (!parser.areThereMsgs())
                Assert.fail("No messages found");
 
            String msgs[] = parser.getMsgs();
            if (msgs.length > 1)
                Assert.fail("More than one message found");
            else
                Assert.assertEquals(xml1 + xml2, msgs[0]);
        } catch (Exception e) {
            Assert.fail(e.getMessage());
        }
    }

Feel free to download the full project source code.

Posted in Design, Programming, Testing | No Comments »

Refactoring Teaser IV Solution

Sunday, September 27th, 2009

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

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

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

28
29
30

private final User naresh_from_mumbai = new User("naresh", "jains", "mumbai", "india", "indian");
private final Context lets = new Context(userService, dns);
private final EmailSuggester suggester = new EmailSuggester(userService, dns, randomNumberGenerator);

@Test
public void suggestIdsUsingNameLocationAndNationality() {
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("naresh@jains.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
}

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

@Test
public void avoidCelebrityNamesInGeneratedIds() {
    lets.assume("naresh", "jains").isACelebrityName();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("nares@jain.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
}

@Test
public void appendCurrentYearWithFirstNameIfIdIsNotAvailable() {
    lets.assume().identity("naresh@jains.com").isNotAvailable();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("naresh2009@jains.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
}

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

public List<String> optionsFor(final User user) {
    List<String> ids = new ArrayList<String>();
    List<String> variations = asList(user.lastName, user.countryName, user.countryMoniker, user.city);
    for (String variation : variations) {
        UserData data = new UserData(user.firstName, variation, user.lastName);
        data.addGeneratedIdTo(ids);
    }
    return ids;
}

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

private void addGeneratedIdTo(final List<String> ids) {
    for (EmailData potential : buildAllPotentialEmailCombinations()) {
        String email = Email.create(potential.userName, potential.domain, dns);
        if (userService.isEmailAvailable(email)) {
            ids.add(email);
            break;
        }
    }
}

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

private List<EmailData> getAllPotentialEmailCombinations() {
    return new ArrayList<EmailData>() {
        {
            add(new EmailData(firstName, seed));
 
            if (seed != lastName) {
                add(new EmailData((firstName + lastName), seed));
                add(new EmailData((firstName + lastName.charAt(0)), seed));
            }
 
            add(new EmailData((firstName + currentYear()), seed));
 
            if (seed != lastName)
                add(new EmailData((firstName + lastName.charAt(0) + currentYear()), seed));
 
            for (int i = 0; i < MAX_RETRIES_FOR_RANDOM_NUMBER; ++i)
                add(new EmailData((firstName + randomNumber.next()), seed));
        }
    };
}

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

if (seed != lastName) {

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

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

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

private String buildIdWithoutRestrictedWordsAndCelebrityNames() {
    Email current = this;
    if (isCelebrityName())
        current = trimLastCharacter();
    return buildIdWithoutRestrictedWordsAndCelebrityNames(current, 1);
}

private String buildIdWithoutRestrictedWordsAndCelebrityNames(final Email last, final int count) {
    if (count == MAX_ATTEMPTS)
        throw new IllegalStateException("Exceeded the Max number of tries");
    String userName = findClosestNonRestrictiveWord(last.userName, RestrictedUserNames, 0);
    String domainName = findClosestNonRestrictiveWord(last.domainName, RestrictedDomainNames, 0);
    Email id = new Email(userName, domainName, dns);
    if (!id.isCelebrityName())
        return id.asString();
    return buildIdWithoutRestrictedWordsAndCelebrityNames(id.trimLastCharacter(), count + 1);
}

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

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

public interface RestrictedWords {
    RestrictedWords RestrictedUserNames = new RestrictedWords() {
        @Override
        public boolean contains(final String word, final DomainNameService dns) {
            return dns.isRestrictedUserName(word);
        }
    };
 
    RestrictedWords RestrictedDomainNames = new RestrictedWords() {
        @Override
        public boolean contains(final String word, final DomainNameService dns) {
            return dns.isRestrictedDomainName(word);
        }
    };
 
    boolean contains(final String word, DomainNameService dns);
}

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

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

Posted in Design, Functional, Programming, Testing | No Comments »

Refactoring Teaser IV Solution Summary Report

Sunday, September 27th, 2009

Topic	Before	After
Project Size	Production Code Package =4 Classes =30 Methods = 90 (average 3/class) LOC = 480 (average 5.33/method and 16/class) Average Cyclomatic Complexity/Method = 1.89 Test Code Package =3 Classes = 19 Methods = 106 LOC = 1379	Production Code Package = 2 Classes =7 Methods = 24 (average 3.43/class) LOC = 168 (average 6.42/method and 18.56/class) Average Cyclomatic Complexity/Method = 1.83 Test Code Package = 1 Classes = 4 Methods = 53 LOC =243
Code Coverage	Line Coverage: 88% Block Coverage: 89%	Line Coverage: 95% Block Coverage: 94%
Cyclomatic Complexity
Coding Convention Violation	85	0

Posted in Design, Programming, Testing | No Comments »

Refactoring Teaser IV – Part 2

Tuesday, August 18th, 2009

Time to take the next baby step.

Lets draw our attention to:

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

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

Black Sheep
Refused Bequest
Dumb Data Holder

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

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

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

And now the code:

public class SuggestionsUtil {
    private static int MAX_ATTEMPTS = 5;
    private final DomainNameService domainNameService;
 
    public SuggestionsUtil(final DomainNameService domainNameService) {
        this.domainNameService = domainNameService;
    }

public IDTokens getIdentityTokens(String token1, String token2) {
    if (isCelebrityName(token1, token2)) {
        token1 = token1.substring(0, token1.length() - 1);
        token2 = token2.substring(0, token2.length() - 1);
    }
    int loopCounter = 1;
    do {
        loopCounter++;
        String generatedFirstToken = generateFirstToken(token1);
        String generatedSecondToken = generateSecondToken(token2);
        if (generatedFirstToken == null || generatedSecondToken == null)
            return null;
        else if (isCelebrityName(generatedFirstToken, generatedSecondToken)) {
            token1 = generatedFirstToken.substring(0, generatedFirstToken.length() - 1);
            token2 = generatedSecondToken.substring(0, generatedSecondToken.length() - 1);
        } else
            return new IDTokens(generatedFirstToken, generatedSecondToken);
    } while (loopCounter != MAX_ATTEMPTS);
 
    return null;
}

private String generateSecondToken(String token2) {
    int loopCounter = 0;
    String restrictedWord = null;
    do {
        restrictedWord = domainNameService.validateSecondPartAndReturnRestrictedWordIfAny(token2);
        String replacement = null;
        if (restrictedWord != null) {
            replacement = restrictedWord.substring(0, restrictedWord.length() - 1);
            token2 = token2.replaceAll(restrictedWord, replacement);
            loopCounter++;
        }
    } while (restrictedWord != null &amp;&amp; loopCounter != MAX_ATTEMPTS);
 
    if (loopCounter == MAX_ATTEMPTS)
        return null;
    return token2;
}

private String generateFirstToken(String token1) {
 
    int loopCounter = 0;
    String restrictedWord = null;
    do {
        restrictedWord = domainNameService.validateFirstPartAndReturnRestrictedWordIfAny(token1);
        String replacement = null;
        if (restrictedWord != null) {
            replacement = restrictedWord.substring(0, restrictedWord.length() - 1);
            token1 = token1.replaceAll(restrictedWord, replacement);
            loopCounter++;
        }
    } while (restrictedWord != null &amp;&amp; loopCounter != MAX_ATTEMPTS);
 
    if (loopCounter == MAX_ATTEMPTS)
        return null;
    return token1;
}

private boolean isCelebrityName(final String token1, final String token2) {
    return domainNameService.isCelebrityName(token1, token2);
}
 
public String appendTokensForId(final String token1, final String token2) {
    return token1.toLowerCase().concat("@").concat(token2.toLowerCase()).concat(".com");
}

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

Download the Source Code here: Java or C#.

Posted in Agile, Design, Programming | No Comments »

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("naresh@jain.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@india.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@indian.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@mumbai.com")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("naresh@jain.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
 
    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("nares@jain.com")).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("nares@india.com")).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("nares@indian.com")).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("nares@mumbai.com")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("nares@jain.com", "nares@india.com", "nares@indian.com", "nares@mumbai.com");
 
    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("nares@jai.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@india.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@indian.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@mumbai.com")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("nares@jai.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
 
    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("naresh@jain.com")).andStubReturn(false);
 
    expect(domainNameService.isCelebrityName("Naresh2009", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh2009")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh2009@jain.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@india.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@indian.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@mumbai.com")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("naresh2009@jain.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
 
    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("naresh@jain.com")).andStubReturn(false);
 
    expect(domainNameService.isCelebrityName("Naresh2009", "Jain")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh2009")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh2009@jain.com")).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("naresh_random@jain.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@india.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@indian.com")).andStubReturn(true);
 
    expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
    expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
    expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
    expect(userService.isIdentityAvailable("naresh@mumbai.com")).andStubReturn(true);
 
    replay(userService, domainNameService);
 
    SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
    List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
    List expectedIds = ids("naresh_random@jain.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
 
    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#.

Posted in Agile, Design, Programming, Testing | No Comments »

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.

Posted in Agile, Design, Programming, Testing | 6 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 »

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
                    &amp;&amp; (rOBj.getReceiveByIpAddress().equals(rOBj.getReceiveFromIpAddress()) || "127.0.0.1".equals(rOBj
                            .getReceiveFromIpAddress()))) {
                continue;
            }
 
            if (mxRecords[0].toLowerCase() == rOBj.getReceiveByHeader().toLowerCase()) {
                if (VerifyDomain(rOBj.getReceiveFromHeader()) &amp;&amp; !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()) &amp;&amp; !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() &amp;&amp; rOBJMatch.find())) {
                    continue;
                }
 
                if (mxRecordMatch.group(2).toLowerCase() == rOBJMatch.group(2).toLowerCase()) {
                    if (VerifyDomain(rOBj.getReceiveFromHeader()) &amp;&amp; !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 &amp;&amp; senderDomain.trim().length() &gt; 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() &gt; 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 &amp;&amp; senderDomain.trim().length() &gt; 0) {
            if (!(senderDomain.length() == 0 || senderDomain.length() &gt; 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 &amp;&amp; ipAddress.trim().length() &gt; 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.

Posted in Agile, Design, Programming | 2 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 | No Comments »