Everything else is just Noise
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.
