» code smells » Managed Chaos by Naresh Jain

Primitive Obsession

Tuesday, October 20th, 2009

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

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

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

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

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

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

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

Posted in Design | No Comments »

Biggest Stinkers

Monday, October 19th, 2009

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

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

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

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

Corey Haines suggested, Duplicate Code
I suggested, Primitive Obsession (Dealing with low level data structures/data types when higher order abstractions can reduce complexity n-fold. This is not specific to OO. Its about lack of abstractions at the right level)
Matt Van Vleet suggested, Single Responsibility Principle
Venkat Subramaniam suggested, Don’t write code
Jim Weirich (was not present at the conference) suggests, Connascence

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

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

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

Posted in Conference, Design | 4 Comments »

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

/*
 * 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>");
}

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

/*
 * @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:

    @Override
    protected void setUp() throws Exception {
        super.setUp();
        // Create parser
        parser = new LightWeightXMLParser(CHARSET);
        // Crete byte buffer and append text
        in = ByteBuffer.allocate(4096);
    }

    @Override
    protected void tearDown() throws Exception {
        super.tearDown();
        // Release byte buffer
        in.clear();
    }

    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]);
    }

    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]);
    }

    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]);
    }

    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]);
    }

    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);
    }

    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]);
    }

    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());
    }

    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]);
    }

    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]);
    }

    public void testStanzaWithComments() throws Exception {
        String msg1 = "<iq from=\"[email protected]/spark\"><query xmlns=\"jabber:iq:privacy\"><!-- silly comment --></query></iq>";
        in.put(msg1.getBytes());
        in.flip();
        // Fill parser with byte buffer content and parse it
        parser.read(in);
        // Make verifications
        assertTrue("No messages were found in stanza", parser.areThereMsgs());
        String[] values = parser.getMsgs();
        assertEquals("Wrong number of parsed stanzas", 1, values.length);
        assertEquals("Wrong stanza was parsed", msg1, values[0]);
    }

    public void testWeirdoContent() throws Exception {
        final String[] testStanzas = { "<?xml version=\"1.0\"?>",
                "<stream:stream xmlns:stream=\"http://etherx.jabber.org/streams\" xmlns=\"jabber:client\" to=\"localhost\" >",
                "<emppartag test=\"1\"/>", "<cdatatest><![CDATA[just<ignore everything& >>here<<<<< /> />]]&gt;]]></cdatatest>",
                "<esctest param=\"1\"> this \" is / a test /> test /> </esctest>",
                "<comtest>this <!-- comment --> is a comment</comtest>", "<emptag/>",
                "<iq type=\"get\" id=\"aab1a\" ><query xmlns=\"jabber:iq:roster\"/> <tag> text </tag></iq>",
                "<iq type=\"get\" id=\"aab1a\" ><query xmlns=\"jabber:iq:roster\"/> </iq>",
                "<message><body xmlns=\"http://idetalk.com/namespace\">12\"</body></message>",
                "<message to=\"[email protected]\" id=\"XRk8p-X\"><body> /> /> </body></message>", };
        String testMsg = "";
        for (String s : testStanzas)
            testMsg += s;
        ByteBuffer mybuffer = ByteBuffer.wrap(testMsg.getBytes());
        parser.read(mybuffer);
 
        String[] msgs = parser.getMsgs();
        for (int i = 0; i < testStanzas.length; i++) {
            assertTrue(i < msgs.length);
            assertEquals(testStanzas[i], msgs[i]);
        }
    }

    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.

Posted in Design, Programming, Testing | No Comments »

Redefining Legacy Code

Wednesday, September 16th, 2009

Michael Feathers did a great job by redefining legacy code to: “Code without Tests”.

Over the years, I’ve dealt with code which had tests (unit, functional or both). Some of it was even test driven. But it was extremely difficult to understand and maintain the code. The code-base exhibited the same problems as Legacy code.

What does this mean? IMHO, it means we need to broaden our definition of Legacy Code.

Legacy code is code that developers fear facing. Legacy code does not communicate its intent and has a very convoluted design. It is code with high viscosity which encourages sloppy job by the developers and makes it extremely difficult for them to do the right thing. Abundance of Code Smells, lack of Tests, long feedback cycles, unpredictability, etc : all of these are contributing factors.

Posted in Programming, Testing | 2 Comments »

Are comments Evil?

Wednesday, August 19th, 2009

Today @Directi we had a few freshers from Universities come down for an interview. As part of the interview process, I presented on Code Smells. During this I was thrashing the whole “write good comments” universal best practice.

During this Ramki was sitting and he silently wrote me an email asking “Are comments evil?” His email follows:

Naresh,

Can you please opine if comments are bad in the following scenario:
(For this project dojo is a dependency and is not owned/maintained by the guys using it to write this code)

this.show = function(){
    this.view.show();
    //this is a hack, dojo seem to have an issue in rendering
    //dynamically/programmatically generated widgets.
    //We *should* call resize() to ensure that the widget is appropriately rendered
    this.view.resize();
}

—- Vs. ——-

this.show = function(){
    this.view.show();
    this.view.resize();
}

In the later case where comments are removed or say never written, though the user understands that you are resizing he would still be wondering why am I calling resize() when I am just showing..?

This is a great question Ramki.

I always say that comments are evil and we should not write comments. Also the first thing I do when I see some code is delete all the comments in it. This is an over generalized comment.

What I really mean:

Writing comments that explain “how” or “what” is evil. Comments (esp. about what and how) is a clear failure to express the intent in code. Comment is a deodorant to hide that failure (smell). However sometimes “the why” is not apparent and if you don’t find a suitable way to communicate that through code, comment is the fall back option. Note that comments are a fall back option rather than a default option. More on this…document the why.

At times, one has to think hard to write code that expresses intent rather than write some sloppy code with poor abstractions and get away by writing comments.

In my Self Documenting Code blog I show a similar example and explain the thinking process.

In case of your code I could write it as follows:

this.show = function(){
    this.view.show();
    reRenderDynamicallyGeneratedWidgets_dojoHack(this.view);
}
 
function reRenderDynamicallyGeneratedWidgets_dojoHack(view){
    // Link describing this bug and the workaround.
    view.resize();
}

This is really pushing it, but I hope you understand where I’m heading with this.

Posted in Agile, Design, Programming | 16 Comments »

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

Posted in Agile, Design, Programming | No Comments »

Killing Speculative Generality Code Smell

Friday, June 26th, 2009

I’m just reviewing a project’s code. I found a common pattern used in their code base. Every class implements an Interface. Each interface is only implemented by one class. Even more interesting, this interface is not exposed outside. In other words, its not exposed as part of the API.

Then my question is

Why do we need the interface? Why can’t we just use the class directly?

Apparently there is no valid answer. Some told me,

Spring forces you to have interfaces.
- That’s not true.
Some told me their mocking framework does not support mocking a class.
- This is also not true. Most mocking frameworks come with a class extension. Some new frameworks, don’t even distinguish between an interface and a class.

Anyway, we don’t need one stupid interface for every class we create. YAGNI. When we need it, we’ll create it. This is one form of speculative generality code smell.

Go ahead, kill it!

Posted in Agile, Design, Programming | 8 Comments »

Code Smells or Code Screams?

Thursday, March 19th, 2009

According to Joshua Kerievsky

Code Smells identify frequently occurring design problems in a way that is more specific or targeted than general design guidelines (like “loosely coupled code” or “duplication-free code”).

The term Code Smell was originally coined by Kent Beck and Martin‘s Refactoring book made it really big. I completely dig the whole “Smell” analogy.

But of late, Sandeep and I’ve been thinking on lines of Code Screams. Code Smells seems a little subtle to me. The Scream analogy goes inline with “Listen to your Code” advice. Also as Nick pointed out, if you ignore Code Screams for a while, you might go deaf!

Posted in Agile, Design, Programming | No Comments »

Value Objects Aren’t Data Classes

Tuesday, March 3rd, 2009

According to Domain Driven Design: A Value Object is an object that describes some characteristic or attribute but carries no concept of identity.

From C2 Wiki: Examples of value objects are things like numbers, dates, monies and strings. Usually, they are small objects which are used quite widely. Their identity is based on their state rather than on their object identity.

According to Martin Fowler: So if you design an object that should be a value object, don’t provide any methods that change its state, .i.e. make it immutable.

In the refactoring book, Martin describes a code smell called Data classes. These are classes that have fields, getting and setting methods for the fields, and nothing else. Such classes are dumb data holders and are almost certainly being manipulated in far too much detail by other classes. Data classes are like children. They are okay as a starting point, but to participate as a grownup object, they need to take some responsibility.

So Value Objects don’t have the Data Class smell.

Posted in Agile, Programming | 10 Comments »

Long Method Smell: When is a method too big?

Wednesday, August 20th, 2008

Unfortunately most people still measure size of code in number of Lines of Code (LoC). We all know LoC is a professional malpractice. Now, how do you objectively identify a long method? If we are not supposed to count LoC, then how can we define a long method?

Some people say, if the code does not fit in one screen and if you have to hit page down, then the method is long. How many times have you looked at code that fits in one screen, but still felt that code was long? Happens to me all the time.

Joshua Kerievsky says “If one cannot quickly and easily understand what a method does and how the method does it, it is a long method”. I really like this definition. But is a little wage to me and I don’t quite understand the theory behind why and when can something be hard to easily understand.

One approach I’ve found to rationalize long method smell is by using the Single Responsibility Principle (SRP). If the method violates SRP, there is a good chance that its Long Method.

If I need to parse the method’s code more than once, then its a good indication that the method is complicated to understand.

Cyclomatic Complexity can also give some interesting data points to under/measure when a piece of code is long. Usually large methods have a higher CC.

Recently I stumbled upon “The Magical Number Seven, Plus or Minus Two: Some Limits on Our Capacity for Processing Information“, a 1956 paper by the cognitive psychologist George A. Miller of Princeton University’s Department of Psychology.

In this paper, Miller showed a number of remarkable coincidences between the channel capacity of a number of human cognitive and perceptual tasks. In each case, the effective channel capacity is equivalent to between 5 and 9 equally-weighted error-less choices: on average, about 2.5 bits of information. – Source WikiPedia

What does this mean? In a layman’s world, this means that 7+/-2 is the number of things (concepts) we can hold in our brain. So when I look at a piece of code and if it has more than 9 things in there, it exceeds my brain capacity to hold it in my memory and actually understand what is going on. I often notice that 7 or less things in the code is easy to manage. Once it starts cross that number, its gets exponentially difficult to hold it in my mind and to understand what is going on.

So if you are thinking of deleting elements from an array if they match a set of to-be-deleted elements, then that’s a good method for me. Why? Coz : I have an array, a set, an iterator, a loop, current values, a comparator and a delete operation. Around 7 things. That’s the max I can hold in my brain. But now if all of a sudden you throw thread synchronization into this, I may end up taking the loop, matching the current elements and the deletion out into another method.

So size has nothing to do with LoC, its a measure of related concepts that you need to hold in your brain.

Posted in Agile, Design, legacy code, Programming | 2 Comments »