Agile FAQs
  About   Slides   Home  

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

Primitive Obsession

Tuesday, October 20th, 2009

When you smell complexity and lack of clarity in the air, look around, you’ll find your code swimming in a (smelly) soup of primitives (low level data-types, functions and language components). Unable to bare the stink, your code is screaming and screeching, asking you to rescue it.

This is my friend, primitive obsession, the stinkiest code smell. You can rescue your code (yes we can) by creating higher level abstractions (functions, data types, objects) and giving some sense to this anarchy.

Primitive Obsession

Primitive Obsession is about lack of abstractions. In the OO world, Methods, Objects, Packages/Namespaces are ways of creating abstraction. Similarly functions, procedures, modules, etc are also valid ways of creating abstractions.

Adding more objects does not always lead to better abstraction. Sometimes removing objects is more useful.

There are many different refactorings that can be used as a remedies:

  • Extract Class
  • Replace Data Value with Object
  • Replace Type Code with Class
  • Introduce Parameter Object
  • Replace Array with Object

One of my favorite example of Primitive Obsession (before and after).

An Example of Primitive Obsession

Tuesday, October 20th, 2009

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

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

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

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

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

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

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

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

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

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

The second method is:

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

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

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

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

This is an example of Primitive Obsession.

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

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

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

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

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

Biggest Stinkers

Monday, October 19th, 2009

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

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

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

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

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

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

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

Refactoring Teaser IV – 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("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#.

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 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.

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 > 1) {
        result *= currentOperandValue;
        --currentOperandValue;
      }
      return result;
    }
  };
 
  public abstract void eval(final OperandStack stack);
}

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

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

and

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