Agile FAQs
  About   Slides   Home  

 
Managed Chaos
Naresh Jain’s Random Thoughts on Software Development and Adventure Sports
     
`
 
Discovering...
Industrial Logic

Microblog Feed
    Previous Feeds...
    Recent Thoughts

    Recent Comments
    Categories
    Archives
    March 2010
    M T W T F S S
    « Feb    
    1234567
    891011121314
    15161718192021
    22232425262728
    293031  
    RSS Feed
    Add to Technorati Favorites

    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=\"lg@jabber.org/spark\"><query xmlns=\"jabber:iq:privacy\"><!-- silly comment --></query></iq>";
            in.put(msg1.getBytes());
            in.flip();
            // Fill parser with byte buffer content and parse it
            parser.read(in);
            // Make verifications
            assertTrue("No messages were found in stanza", parser.areThereMsgs());
            String[] values = parser.getMsgs();
            assertEquals("Wrong number of parsed stanzas", 1, values.length);
            assertEquals("Wrong stanza was parsed", msg1, values[0]);
        }
    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=\"lg@jabber.org\" id=\"XRk8p-X\"><body> /> /> </body></message>", };
            String testMsg = "";
            for (String s : testStanzas)
                testMsg += s;
            ByteBuffer mybuffer = ByteBuffer.wrap(testMsg.getBytes());
            parser.read(mybuffer);
     
            String[] msgs = parser.getMsgs();
            for (int i = 0; i < testStanzas.length; i++) {
                assertTrue(i < msgs.length);
                assertEquals(testStanzas[i], msgs[i]);
            }
        }
    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.

    • Share/Bookmark

    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("naresh@jains.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
    }
    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("nares@jains.com", "nares@india.com", "nares@indian.com", "nares@mumbai.com");
    }
    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("nares@jain.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
    }
    52
    53
    54
    55
    56
    57
    
    @Test
    public void appendCurrentYearWithFirstNameIfIdIsNotAvailable() {
        lets.assume().identity("naresh@jains.com").isNotAvailable();
        List<String> suggestions = suggester.optionsFor(naresh_from_mumbai);
        lets.assertThat(suggestions).are("naresh2009@jains.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
    }

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

    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.

    • Share/Bookmark

    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
    • Share/Bookmark

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

    • Share/Bookmark

    Refactoring Teaser IV - Step 1

    Wednesday, August 12th, 2009

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

    Download the Source Code here: Java or C#.

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

    Test Setup:

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

    First Test (Happy Path)

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

    Second Test

    @Test
    public void avoidRestrictedWordsInIds() {
        expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn("Naresh");
     
        expect(domainNameService.isCelebrityName("Nares", "Jain")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
        expect(userService.isIdentityAvailable("nares@jain.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Nares", "India")).andStubReturn(false);
        expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
        expect(userService.isIdentityAvailable("nares@india.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Nares", "Indian")).andStubReturn(false);
        expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
        expect(userService.isIdentityAvailable("nares@indian.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Nares", "Mumbai")).andStubReturn(false);
        expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
        expect(userService.isIdentityAvailable("nares@mumbai.com")).andStubReturn(true);
     
        replay(userService, domainNameService);
     
        SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
        List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
        List expectedIds = ids("nares@jain.com", "nares@india.com", "nares@indian.com", "nares@mumbai.com");
     
        assertEquals(expectedIds, generatedIDs);
     
        verify(userService, domainNameService);
    }
    @Test
    public void avoidCelebrityNamesInGeneratedIds() {
        expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Nares", "Jai")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Nares")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jai")).andStubReturn(null);
        expect(userService.isIdentityAvailable("nares@jai.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@india.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@indian.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@mumbai.com")).andStubReturn(true);
     
        replay(userService, domainNameService);
     
        SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
        List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
        List expectedIds = ids("nares@jai.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
     
        assertEquals(expectedIds, generatedIDs);
     
        verify(userService, domainNameService);
    }
    @Test
    public void appendCurrentYearWithFirstNameIfIdIsNotAvailable() {
        expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@jain.com")).andStubReturn(false);
     
        expect(domainNameService.isCelebrityName("Naresh2009", "Jain")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh2009")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh2009@jain.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@india.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@indian.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@mumbai.com")).andStubReturn(true);
     
        replay(userService, domainNameService);
     
        SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
        List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
        List expectedIds = ids("naresh2009@jain.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
     
        assertEquals(expectedIds, generatedIDs);
     
        verify(userService, domainNameService);
    }
    @Test
    public void appendRandomNumberWithFirstNameIfIdIsNotAvailable() {
        expect(domainNameService.isCelebrityName("Naresh", "Jain")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@jain.com")).andStubReturn(false);
     
        expect(domainNameService.isCelebrityName("Naresh2009", "Jain")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh2009")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh2009@jain.com")).andStubReturn(false);
     
        expect(domainNameService.isCelebrityName("Naresh_random", "Jain")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh_random")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Jain")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh_random@jain.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "India")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("India")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@india.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "Indian")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Indian")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@indian.com")).andStubReturn(true);
     
        expect(domainNameService.isCelebrityName("Naresh", "Mumbai")).andStubReturn(false);
        expect(domainNameService.validateFirstPartAndReturnRestrictedWordIfAny("Naresh")).andStubReturn(null);
        expect(domainNameService.validateSecondPartAndReturnRestrictedWordIfAny("Mumbai")).andStubReturn(null);
        expect(userService.isIdentityAvailable("naresh@mumbai.com")).andStubReturn(true);
     
        replay(userService, domainNameService);
     
        SuggestionParam suggestionParam = new SuggestionParam(location, "Naresh", "Jain");
        List generatedIDs = identityGenerator.getGeneratedIDs(suggestionParam);
        List expectedIds = ids("naresh_random@jain.com", "naresh@india.com", "naresh@indian.com", "naresh@mumbai.com");
     
        assertEquals(expectedIds, generatedIDs);
     
        verify(userService, domainNameService);
    }

    Some helper method:

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

    Download the Source Code here: Java or C#.

    • Share/Bookmark

    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.

    • Share/Bookmark

    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.

    • Share/Bookmark

    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.

    • Share/Bookmark

    Refactoring Teaser 1: Take 1

    Tuesday, July 14th, 2009

    Last week I posted a small code snippet for refactoring under the heading Refactoring Teaser.

    In this post I’ll try to show step by step how I would try to refactor this mud ball.

    First and foremost cleaned up the tests to communicate the intent. Also notice I’ve changed the test class name to ContentTest instead of StringUtilTest, which means anything and everything.

    public class ContentTest {
        private Content helloWorldJava = new Content("Hello World Java");
        private Content helloWorld = new Content("Hello World!");
     
        @Test
        public void ignoreContentSmallerThan3Words() {
            assertEquals("", helloWorld.toString());
        }
     
        @Test
        public void buildOneTwoAndThreeWordPhrasesFromContent() {
            assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", helloWorldJava.toPhrases(6));
        }
     
        @Test
        public void numberOfOutputPhrasesAreConfigurable() {
            assertEquals("'Hello'", helloWorldJava.toPhrases(1));
            assertEquals("'Hello', 'World', 'Java', 'Hello World'", helloWorldJava.toPhrases(4));
        }
     
        @Test
        public void returnsAllPhrasesUptoTheNumberSpecified() {
            assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", helloWorldJava.toPhrases(10));
        }
    }

    Next, I created a class called Content, instead of StringUtil. Content is a first-class domain object. Also notice, no more side-effect intense statics.

    public class Content {
        private static final String BLANK_OUTPUT = "";
        private static final String SPACE = " ";
        private static final String DELIMITER = "', '";
        private static final String SINGLE_QUOTE = "'";
        private static final int MIN_NO_WORDS = 2;
        private static final Pattern ON_WHITESPACES = Pattern.compile("\\p{Z}|\\p{P}");
        private List phrases = new ArrayList();
     
        public Content(final String content) {
            String[] tokens = ON_WHITESPACES.split(content);
            if (tokens.length &gt; MIN_NO_WORDS) {
                buildAllPhrasesUptoThreeWordsFrom(tokens);
            }
        }
     
        @Override
        public String toString() {
            return toPhrases(Integer.MAX_VALUE);
        }
     
        public String toPhrases(final int userRequestedSize) {
            if (phrases.isEmpty()) {
                return BLANK_OUTPUT;
            }
            List requiredPhrases = phrases.subList(0, numberOfPhrasesRequired(userRequestedSize));
            return withInQuotes(join(requiredPhrases, DELIMITER));
        }
     
        private String withInQuotes(final String phrases) {
            return SINGLE_QUOTE + phrases + SINGLE_QUOTE;
        }
     
        private int numberOfPhrasesRequired(final int userRequestedSize) {
            return userRequestedSize &gt; phrases.size() ? phrases.size() : userRequestedSize;
        }
     
        private void buildAllPhrasesUptoThreeWordsFrom(final String[] words) {
            buildSingleWordPhrases(words);
            buildDoubleWordPhrases(words);
            buildTripleWordPhrases(words);
        }
     
        private void buildSingleWordPhrases(final String[] words) {
            for (int i = 0; i &lt; words.length; ++i) {
                phrases.add(words[i]);
            }
        }
     
        private void buildDoubleWordPhrases(final String[] words) {
            for (int i = 0; i &lt; words.length - 1; ++i) {
                phrases.add(words[i] + SPACE + words[i + 1]);
            }
        }
     
        private void buildTripleWordPhrases(final String[] words) {
            for (int i = 0; i &lt; words.length - 2; ++i) {
                phrases.add(words[i] + SPACE + words[i + 1] + SPACE + words[i + 2]);
            }
        }
    }

    This was a big step forward, but not good enough. Next I focused on the following code:

        private void buildAllPhrasesUptoThreeWordsFrom(final String[] words) {
            buildSingleWordPhrases(words);
            buildDoubleWordPhrases(words);
            buildTripleWordPhrases(words);
        }
     
        private void buildSingleWordPhrases(final String[] words) {
            for (int i = 0; i &lt; words.length; ++i) {
                phrases.add(words[i]);
            }
        }
     
        private void buildDoubleWordPhrases(final String[] words) {
            for (int i = 0; i &lt; words.length - 1; ++i) {
                phrases.add(words[i] + SPACE + words[i + 1]);
            }
        }
     
        private void buildTripleWordPhrases(final String[] words) {
            for (int i = 0; i &lt; words.length - 2; ++i) {
                phrases.add(words[i] + SPACE + words[i + 1] + SPACE + words[i + 2]);
            }
        }

    The above code violates the Open-Closed Principle (pdf). It also smells of duplication. Created a somewhat generic method to kill the duplication.

        private void buildAllPhrasesUptoThreeWordsFrom(final String[] fromWords) {
            buildPhrasesOf(ONE_WORD, fromWords);
            buildPhrasesOf(TWO_WORDS, fromWords);
            buildPhrasesOf(THREE_WORDS, fromWords);
        }
     
        private void buildPhrasesOf(final int phraseLength, final String[] tokens) {
            for (int i = 0; i &lt;= tokens.length - phraseLength; ++i) {
                String phrase = phraseAt(i, tokens, phraseLength);
                phrases.add(phrase);
            }
        }
     
        private String phraseAt(final int currentIndex, final String[] tokens, final int phraseLength) {
            StringBuilder phrase = new StringBuilder(tokens[currentIndex]);
            for (int i = 1; i &lt; phraseLength; i++) {
                phrase.append(SPACE + tokens[currentIndex + i]);
            }
            return phrase.toString();
        }

    Now I had a feeling that my Content class was doing too much and also suffered from the primitive obsession code smell. Looked like a concept/abstraction (class) was dying to be called out. So created a Words class as an inner class.

        private class Words {
            private String[] tokens;
            private static final String SPACE = " ";
     
            Words(final String content) {
                tokens = ON_WHITESPACES.split(content);
            }
     
            boolean has(final int minNoWords) {
                return tokens.length &gt; minNoWords;
            }
     
            List phrasesOf(final int length) {
                List phrases = new ArrayList();
                for (int i = 0; i &lt;= tokens.length - length; ++i) {
                    String phrase = phraseAt(i, length);
                    phrases.add(phrase);
                }
                return phrases;
            }
     
            private String phraseAt(final int index, final int length) {
                StringBuilder phrase = new StringBuilder(tokens[index]);
                for (int i = 1; i &lt; length; i++) {
                    phrase.append(SPACE + tokens[index + i]);
                }
                return phrase.toString();
            }
        }

    In the constructor of the Content class we instantiate a Words class as follows:

        public Content(final String content) {
            Words words = new Words(content);
            if (words.has(MIN_NO_WORDS)) {
                phrases.addAll(words.phrasesOf(ONE_WORD));
                phrases.addAll(words.phrasesOf(TWO_WORDS));
                phrases.addAll(words.phrasesOf(THREE_WORDS));
            }
        }

    Even though this code communicates well, there is duplication and noise that can be removed without compromising on the communication.

         phrases.addAll(words.phrasesOf(ONE_WORD, TWO_WORDS, THREE_WORDS));

    There are few more version after this, but I think this should give you an idea about the direction I’m heading.

    • Share/Bookmark

    Refactoring Teaser Part 1

    Wednesday, July 8th, 2009

    How would you refactoring the following code? (This code is in Java, but you can refactoring using any language of your choice).

    Following test explains the functionality of the production code:

    1
    2
    3
    4
    5
    6
    7
    8
    9
    
    public class StringUtilTest {
      @Test
      public void testSplit() {
        assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", StringUtil.split("Hello World Java", 6));
        assertEquals("'Hello', 'World', 'Java', 'Hello World', 'World Java', 'Hello World Java'", StringUtil.split("Hello World Java", 10));
        assertEquals("'Hello', 'World', 'Java', 'Hello World'", StringUtil.split("Hello World Java", 4));
        assertEquals("'Hello'", StringUtil.split("Hello World Java", 1));
      }
    }

    General use case is that for a given string (content), users might want split the same string to get different numbers of keywords in the output.

    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
    38
    39
    40
    41
    42
    43
    44
    45
    46
    47
    48
    49
    50
    51
    52
    53
    54
    55
    56
    
    public final class StringUtil {
      private static final Pattern REGEX_TO_SPLIT_ALONG_WHITESPACES = Pattern.compile("\\p{Z}|\\p{P}");
     
      public static String split(final String content, final int number) {
        String listOfKeywords = "";
        int count = 0;
        String[] tokens = REGEX_TO_SPLIT_ALONG_WHITESPACES.split(content);
        List<string> strings = Arrays.asList(tokens);
        List<string> allStrings = singleDoubleTripleWords(strings);
        int size = allStrings.size();
        for (String phrase : allStrings) {
          if (count == number) {
            break;
          }
          listOfKeywords += "'" + phrase + "'";
          if (++count < size && count < number) {
            listOfKeywords += ", ";
          }
        }
        return listOfKeywords;
      }
     
      private static List<String> singleDoubleTripleWords(final List<string> strings) {
        List<string> allStrings = new ArrayList<string>();
        int numWords = strings.size();
     
        if (hasEnoughWords(numWords) == false) {
          return allStrings;
        }
     
        // Extracting single words. Total size of words == numWords
     
        // Extracting single-word phrases.
        for (int i = 0; i < numWords; ++i) {
          allStrings.add(strings.get(i));
        }
     
        // Extracting double-word phrases
        for (int i = 0; i < numWords - 1; ++i) {
          allStrings.add(strings.get(i) + " " + strings.get(i + 1));
        }
     
        // Extracting triple-word phrases
        for (int i = 0; i < numWords - 2; ++i) {
          allStrings.add(strings.get(i) + " " + strings.get(i + 1) + " " + strings.get(i + 2));
        }
        return allStrings;
      }
     
      private static boolean hasEnoughWords(final int numWords) {
        if (numWords < 3) {
          return false;
        }
        return true;
      }
    }
    • Share/Bookmark
        Licensed under
    Creative Commons License
    Design by vikivix