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

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.

Posted in Conference, Design | 4 Comments »

Everything else is just Noise

Tuesday, September 22nd, 2009

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

When I started I had:

1
2
3
4
5
6
7
8
9
10

private final UserService userService = createMock(UserService.class);
private final DomainNameService dns = createMock(DomainNameService.class);
private final RandomNumberGenerator randomNumberGenerator = new RandomNumberGenerator() {
    @Override
    public long next() {
        return 9876543210L;
    }
};
private final IdentityGenerator identityGenerator = new IdentityGenerator(randomNumberGenerator, dns, userService);
private final User naresh_from_mumbai = new User("naresh", "jain", "mumbai", "india", "indian");
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37

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

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

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

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

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

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

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

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

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

1
2
3
4
5
6

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

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

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

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

Posted in Design, Programming, Testing | No Comments »
    Licensed under
Creative Commons License