XNSIO
  About   Slides   Home  

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

Goodbye Simplicity I’m Object Obsessed – Example

Saturday, July 14th, 2012

Suppose we had the following (utterly) simplistic requirement:

@Test
public void politiciansDontNeedToPayTax() {
    whenIncomeIs(10000).verify(POLITICIANS).pay(0);
}
 
@Test
public void residingAliensGetAwayByPayingLittleTax() {
    whenIncomeIs(10000).verify(ALIENS).pay(1000);
}
 
@Test
public void richPeopleArePunishedWithHighestTax() {
    whenIncomeIs(10000).verify(RICH_PEOPLE).pay(3000);
}

where:

TaxPayer RICH_PEOPLE = new RichPeople();
TaxPayer ALIENS = new ResidingAliens();
TaxPayer POLITICIANS = new Politicians();

To fullfil this requirement, we’ve the following code:

Tax Payer

Parent Class:

public abstract class TaxPayer {
    protected abstract double getTaxPercentage();
 
    public double calculateTaxAmount(final long income) {
        if (getTaxPercentage() == 0)
            return 0;
        return income * getTaxPercentage() / 100.0;
    }
}

Each child class:

public class Politicians extends TaxPayer {
    private double taxPercentage = 0;
 
    @Override
    protected double getTaxPercentage() {
        return taxPercentage;
    }
}
public class RichPeople extends TaxPayer {
    private final double taxPercentage = 30;
 
    @Override
    protected double getTaxPercentage() {
        return taxPercentage;
    }
}
public class ResidingAliens extends TaxPayer {
    private final double taxPercentage = 10;
 
    @Override
    protected double getTaxPercentage() {
        return taxPercentage;
    }
}

One would wonder what good are these child classes? Feels like a class explosion. Sure enough! This is the pathetic state of Object Oriented programming in many organizations, esp. the Enterprise software side of the world.

One could have easily used an enum to solve this problem:

Refactored Tax Payer

public enum TaxPayer {
    Politicians(0), Aliens(.1), RichPeople(.3);
 
    private double taxPercentage;
 
    private TaxPayer(double taxPercentage) {
        this.taxPercentage = taxPercentage;
    }
 
    public double calculateTaxAmount(final long income) {
        return income * taxPercentage;
    }
}

Simplifying a Complex Design

Thursday, July 15th, 2010
From

To

Refactoring Teaser V

Thursday, October 1st, 2009

I have a treat for crappy code scavengers. Here is some code which has a Cyclomatic Complexity of 68 and NPath Complexity of 34,632 (this method is ONLY 189 lines long (154 NCSS)).

128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
/*
 * Main reading method
 */
public void read(final ByteBuffer byteBuffer) throws Exception {
    invalidateBuffer();
    // Check that the buffer is not bigger than 1 Megabyte. For security reasons
    // we will abort parsing when 1 Mega of queued chars was found.
    if (buffer.length() > maxBufferSize)
        throw new Exception("Stopped parsing never ending stanza");
    CharBuffer charBuffer = encoder.decode(byteBuffer);
    char[] buf = charBuffer.array();
    int readByte = charBuffer.remaining();
 
    // Just return if nothing was read
    if (readByte == 0)
        return;
 
    // Verify if the last received byte is an incomplete double byte character
    char lastChar = buf[readByte - 1];
    if (lastChar >= 0xfff0) {
        // Rewind the position one place so the last byte stays in the buffer
        // The missing byte should arrive in the next iteration. Once we have both
        // of bytes we will have the correct character
        byteBuffer.position(byteBuffer.position() - 1);
        // Decrease the number of bytes read by one
        readByte--;
        // Just return if nothing was read
        if (readByte == 0)
            return;
    }
 
    buffer.append(buf, 0, readByte);
    // Do nothing if the buffer only contains white spaces
    if (buffer.charAt(0) <= ' ' && buffer.charAt(buffer.length() - 1) <= ' ')
        if ("".equals(buffer.toString().trim())) {
            // Empty the buffer so there is no memory leak
            buffer.delete(0, buffer.length());
            return;
        }
    // Robot.
    char ch;
    boolean isHighSurrogate = false;
    for (int i = 0; i < readByte; i++) {
        ch = buf[i];
        if (ch < 0x20 && ch != 0x9 && ch != 0xA && ch != 0xD && ch != 0x0)
            // Unicode characters in the range 0x0000-0x001F other than 9, A, and D are not allowed in XML
            // We need to allow the NULL character, however, for Flash XMLSocket clients to work.
            throw new Exception("Disallowed character");
        if (isHighSurrogate) {
            if (Character.isLowSurrogate(ch))
                // Everything is fine. Clean up traces for surrogates
                isHighSurrogate = false;
            else
                // Trigger error. Found high surrogate not followed by low surrogate
                throw new Exception("Found high surrogate not followed by low surrogate");
        } else if (Character.isHighSurrogate(ch))
            isHighSurrogate = true;
        else if (Character.isLowSurrogate(ch))
            // Trigger error. Found low surrogate char without a preceding high surrogate
            throw new Exception("Found low surrogate char without a preceding high surrogate");
        if (status == XMLLightweightParser.TAIL) {
            // Looking for the close tag
            if (depth < 1 && ch == head.charAt(tailCount)) {
                tailCount++;
                if (tailCount == head.length()) {
                    // Close stanza found!
                    // Calculate the correct start,end position of the message into the buffer
                    int end = buffer.length() - readByte + i + 1;
                    String msg = buffer.substring(startLastMsg, end);
                    // Add message to the list
                    foundMsg(msg);
                    startLastMsg = end;
                }
            } else {
                tailCount = 0;
                status = XMLLightweightParser.INSIDE;
            }
        } else if (status == XMLLightweightParser.PRETAIL) {
            if (ch == XMLLightweightParser.CDATA_START[cdataOffset]) {
                cdataOffset++;
                if (cdataOffset == XMLLightweightParser.CDATA_START.length) {
                    status = XMLLightweightParser.INSIDE_CDATA;
                    cdataOffset = 0;
                    continue;
                }
            } else {
                cdataOffset = 0;
                status = XMLLightweightParser.INSIDE;
            }
            if (ch == '/') {
                status = XMLLightweightParser.TAIL;
                depth--;
            } else if (ch == '!')
                // This is a <! (comment) so ignore it
                status = XMLLightweightParser.INSIDE;
            else
                depth++;
        } else if (status == XMLLightweightParser.VERIFY_CLOSE_TAG) {
            if (ch == '>') {
                depth--;
                status = XMLLightweightParser.OUTSIDE;
                if (depth < 1) {
                    // Found a tag in the form <tag />
                    int end = buffer.length() - readByte + i + 1;
                    String msg = buffer.substring(startLastMsg, end);
                    // Add message to the list
                    foundMsg(msg);
                    startLastMsg = end;
                }
            } else if (ch == '<') {
                status = XMLLightweightParser.PRETAIL;
                insideChildrenTag = true;
            } else
                status = XMLLightweightParser.INSIDE;
        } else if (status == XMLLightweightParser.INSIDE_PARAM_VALUE) {
 
            if (ch == '"')
                status = XMLLightweightParser.INSIDE;
        } else if (status == XMLLightweightParser.INSIDE_CDATA) {
            if (ch == XMLLightweightParser.CDATA_END[cdataOffset]) {
                cdataOffset++;
                if (cdataOffset == XMLLightweightParser.CDATA_END.length) {
                    status = XMLLightweightParser.OUTSIDE;
                    cdataOffset = 0;
                }
            } else
                cdataOffset = 0;
        } else if (status == XMLLightweightParser.INSIDE) {
            if (ch == XMLLightweightParser.CDATA_START[cdataOffset]) {
                cdataOffset++;
                if (cdataOffset == XMLLightweightParser.CDATA_START.length) {
                    status = XMLLightweightParser.INSIDE_CDATA;
                    cdataOffset = 0;
                    continue;
                }
            } else {
                cdataOffset = 0;
                status = XMLLightweightParser.INSIDE;
            }
            if (ch == '"')
                status = XMLLightweightParser.INSIDE_PARAM_VALUE;
            else if (ch == '>') {
                status = XMLLightweightParser.OUTSIDE;
                if (insideRootTag
                        && ("stream:stream>".equals(head.toString()) || "?xml>".equals(head.toString()) || "flash:stream>".equals(head
                                .toString()))) {
                    // Found closing stream:stream
                    int end = buffer.length() - readByte + i + 1;
                    // Skip LF, CR and other "weird" characters that could appear
                    while (startLastMsg < end && '<' != buffer.charAt(startLastMsg))
                        startLastMsg++;
                    String msg = buffer.substring(startLastMsg, end);
                    foundMsg(msg);
                    startLastMsg = end;
                }
                insideRootTag = false;
            } else if (ch == '/')
                status = XMLLightweightParser.VERIFY_CLOSE_TAG;
        } else if (status == XMLLightweightParser.HEAD) {
            if (ch == ' ' || ch == '>') {
                // Append > to head to allow searching </tag>
                head.append(">");
                if (ch == '>')
                    status = XMLLightweightParser.OUTSIDE;
                else
                    status = XMLLightweightParser.INSIDE;
                insideRootTag = true;
                insideChildrenTag = false;
                continue;
            } else if (ch == '/' && head.length() > 0) {
                status = XMLLightweightParser.VERIFY_CLOSE_TAG;
                depth--;
            }
            head.append(ch);
 
        } else if (status == XMLLightweightParser.INIT) {
            if (ch == '<') {
                status = XMLLightweightParser.HEAD;
                depth = 1;
            } else
                startLastMsg++;
        } else if (status == XMLLightweightParser.OUTSIDE)
            if (ch == '<') {
                status = XMLLightweightParser.PRETAIL;
                cdataOffset = 1;
                insideChildrenTag = true;
            }
    }
    if (head.length() > 0 && ("/stream:stream>".equals(head.toString()) || "/flash:stream>".equals(head.toString())))
        // Found closing stream:stream
        foundMsg("</stream:stream>");
}

What does this code actually do?

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.

86
87
88
89
90
91
92
93
94
95
96
/*
 * @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:

16
17
18
19
20
21
22
23
    @Override
    protected void setUp() throws Exception {
        super.setUp();
        // Create parser
        parser = new LightWeightXMLParser(CHARSET);
        // Crete byte buffer and append text
        in = ByteBuffer.allocate(4096);
    }
25
26
27
28
29
30
    @Override
    protected void tearDown() throws Exception {
        super.tearDown();
        // Release byte buffer
        in.clear();
    }
32
33
34
35
36
37
38
39
40
41
    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]);
    }
43
44
45
46
47
48
49
50
51
52
53
54
55
56
    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]);
    }
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
    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]);
    }
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
    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]);
    }
117
118
119
120
121
122
123
124
125
126
127
    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);
    }
129
130
131
132
133
134
135
136
137
138
139
140
    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]);
    }
142
143
144
145
146
147
148
149
150
    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());
    }
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
    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]);
    }
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
    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]);
    }
196
197
198
199
200
201
202
203
204
205
206
207
    public void testStanzaWithComments() throws Exception {
        String msg1 = "<iq from=\"[email protected]/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]);
    }
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
    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=\"[email protected]\" 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]);
        }
    }
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
    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.

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);
32
33
34
35
36
@Test
public void suggestIdsUsingNameLocationAndNationality() {
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}
38
39
40
41
42
43
@Test
public void avoidRestrictedWordsInIds() {
    lets.assume("naresh").isARestrictedUserName();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}
45
46
47
48
49
50
@Test
public void avoidCelebrityNamesInGeneratedIds() {
    lets.assume("naresh", "jains").isACelebrityName();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}
52
53
54
55
56
57
@Test
public void appendCurrentYearWithFirstNameIfIdIsNotAvailable() {
    lets.assume().identity("[email protected]").isNotAvailable();
    List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
    lets.assertThat(suggestions).are("[email protected]", "[email protected]", "[email protected]", "[email protected]");
}

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

26
27
28
29
30
31
32
33
34
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.

47
48
49
50
51
52
53
54
55
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:

57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
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.

30
31
32
33
34
35
private String buildIdWithoutRestrictedWordsAndCelebrityNames() {
    Email current = this;
    if (isCelebrityName())
        current = trimLastCharacter();
    return buildIdWithoutRestrictedWordsAndCelebrityNames(current, 1);
}
37
38
39
40
41
42
43
44
45
46
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.

5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
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.

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%

Code Coverage Before

  • Line Coverage: 95%
  • Block Coverage: 94%

Code Coverage After

Cyclomatic Complexity Cylcomatic Complexity Before Cylcomatic Complexity After
Coding Convention Violation 85 0

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

Refactoring Teaser IV – Step 1

Wednesday, August 12th, 2009

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

Download the Source Code here: Java or C#.

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

Test Setup:

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

First Test (Happy Path)

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

Second Test

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

Some helper method:

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

Download the Source Code here: Java or C#.

Refactoring Teaser III

Wednesday, August 5th, 2009

This time a simple one.

Following test will help you understand the code:

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

and

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

Feel free to download the Java project.

Refactoring Teaser II: Solution: Take 1

Tuesday, August 4th, 2009

Following is the first take at refactoring the II Teaser.

Started off by refactoring the tests:

@Test
public void mineSenderFromEdgeServerRecordInMailHeaders() {
    header(
            mail_from("67.205.47.130", "agileindia.org").receivedBy("75.119.213.4", "mails.agileindia.org"),
            mail_from(null, "mail-vw0-f172.google.com").receivedBy("67.205.47.130", "agileindia.org"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(2);
}
@Test
public void useSenderIPForInvalidSenderEdgeServerDomainName() {
    header(
            mail_from("67.205.47.130", "agileindia.org").receivedBy("75.119.213.4", "mails.agileindia.org"),
            mail_from("209.85.212.172", "cannot-exist.agilefaqs.com").receivedBy("67.205.47.130", "agileindia.org"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(2);
}
@Test
public void senderNameCanBeIPAddress() {
    header(mail_from(null, "209.85.212.172").receivedBy("67.205.47.130", "agileindia.org"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(1);
}
@Test
public void matchMXRecordIPWithReciever() {
    header(mail_from("", "mail-vw0-f172.google.com").receivedBy("67.205.47.130", "apache2-echo.robin.dreamhost.com"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertThat(senderIP).is("209.85.212.172");
    assertDistanceOfMatchingHeaderFromTopIs(1);
}
@Test
public void skipHeaderRecordsThatDontCrossEdgeServers() {
    header(
            mail_from("192.168.1.47", "smtp.gmail.com").receivedBy("75.119.213.4", "mail.gmail.com"),
            mail_from("192.168.1.3", "192.168.6.242").receivedBy("192.168.1.47", "smtp.gmail.com"));
    senderIP = extractSenderIPFrom(headers).forRecipientDomain(GMAIL);
    assertFalse(senderIP.isValid());
    assertDistanceOfMatchingHeaderFromTopIs(0);
}

Following is the crux of the Sender Edge Server IP Extraction Algo:

public IPAddressExtractor(final List receivedHeaders, final Domain recepientDomain) {
    Domain recepientMXRecord = recepientDomain.retrieveFirstMXRecord();
    for (MatchingCriterion critierion : asList(MATCHING_DOMAIN, MATCHING_IP, MATCHING_SECOND_LEVEL_DOMAIN)) {
        Match result = foreach(receivedHeaders).and(recepientMXRecord).match(critierion);
        if (result.success()) {
            storeSenderIPWithDistance(result);
            break;
        }
    }
}

To do the whole Fluent interfaces on line number 21, I had to create a private method:

private CriterionMatcher foreach(final List mailHeaders) {
    return new CriterionMatcher(mailHeaders);
}

and a package protected CriterionMatcher class

class CriterionMatcher {
    private final List mailHeaders;
    private int counter;
    private Domain mxRecord;
 
    CriterionMatcher(final List mailHeaders) {
        this.mailHeaders = mailHeaders;
    }
 
    CriterionMatcher and(final Domain mxRecord) {
        this.mxRecord = mxRecord;
        return this;
    }
 
    Match match(final MatchingCriterion criterion) {
        for (EmailHeader header : mailHeaders) {
            counter++;
 
            if (criterion.isSatisfiedBy(mxRecord, header)) {
                return new Match(header.fromDomain, header.fromIp, counter);
            }
        }
        return Match.NULL;
    }
}

Other than the switch statement smell and conditional complexity, the original code was obsessed with Primitive Obsession code smell. To fix this issue, the first thing I had to do was great first class citizens (Objects). So I ended up creating

public class IPAddress {
    private static final String IP_ADDRESS_REGEX = "\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b";
    private static final Pattern VALID_IP = Pattern.compile(IP_ADDRESS_REGEX);
    private static final String LOCAL_HOST = "127.0.0.1";
    public static final IPAddress NULL = new IPAddress("");
 
    private final String ip;
 
    private IPAddress(final String address) {
        ip = address;
    }
 
    public static IPAddress parse(final String address) {
        if (address == null) {
            return NULL;
        }
        Matcher matcher = VALID_IP.matcher(address);
 
        if (!matcher.find()) {
            return NULL;
        }
        return new IPAddress(matcher.group(0));
    }
 
    @Override
    public String toString() {
        return ip;
    }
 
    public boolean isLocalhost() {
        return LOCAL_HOST.equals(ip);
    }
 
    public boolean isValid() {
        return this != NULL;
    }
}

and

public class Domain {
    private static final Pattern SLD = Pattern.compile("(.*\\.)?(.*\\..*)");
    public static final Domain NULL = new Domain("", Network.NULL);
 
    private final String name;
    private final Network network;
 
    protected Domain(final String domainName, final Network network) {
        name = domainName.toLowerCase();
        this.network = network;
    }
 
    public IPAddress resolveIP() {
        try {
            String ipAddress = network.ipAddressOf(name);
            return IPAddress.parse(ipAddress);
        } catch (UnknownHostException e) {
            return IPAddress.NULL;
        }
    }
 
    public Domain secondLevelDomain() {
        Matcher mxRecordMatch = SLD.matcher(name);
        if (mxRecordMatch.find()) {
            return new Domain(mxRecordMatch.group(2), network);
        }
        return this;
    }
 
    public Domain retrieveFirstMXRecord() {
        List mxRecords = network.firstMXRecordFor(name);
        if (mxRecords.size() &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.

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.

    Licensed under
Creative Commons License