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:
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):
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)
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.
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
*/publicvoid read(final ByteBuffer byteBuffer)throwsException{
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)thrownewException("Stopped parsing never ending stanza");
CharBuffer charBuffer = encoder.decode(byteBuffer);char[] buf = charBuffer.array();int readByte = charBuffer.remaining();// Just return if nothing was readif(readByte ==0)return;// Verify if the last received byte is an incomplete double byte characterchar 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 readif(readByte ==0)return;}
buffer.append(buf, 0, readByte);// Do nothing if the buffer only contains white spacesif(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.thrownewException("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 surrogatethrownewException("Found high surrogate not followed by low surrogate");}elseif(Character.isHighSurrogate(ch))
isHighSurrogate =true;elseif(Character.isLowSurrogate(ch))// Trigger error. Found low surrogate char without a preceding high surrogatethrownewException("Found low surrogate char without a preceding high surrogate");if(status == XMLLightweightParser.TAIL){// Looking for the close tagif(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 bufferint 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;}}elseif(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--;}elseif(ch =='!')// This is a <! (comment) so ignore it
status = XMLLightweightParser.INSIDE;else
depth++;}elseif(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;}}elseif(ch =='<'){
status = XMLLightweightParser.PRETAIL;
insideChildrenTag =true;}else
status = XMLLightweightParser.INSIDE;}elseif(status == XMLLightweightParser.INSIDE_PARAM_VALUE){if(ch =='"')
status = XMLLightweightParser.INSIDE;}elseif(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;}elseif(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;elseif(ch =='>'){
status = XMLLightweightParser.OUTSIDE;if(insideRootTag
&&("stream:stream>".equals(head.toString())||"?xml>".equals(head.toString())||"flash:stream>".equals(head
.toString()))){// Found closing stream:streamint end = buffer.length()- readByte + i +1;// Skip LF, CR and other "weird" characters that could appearwhile(startLastMsg < end &&'<'!= buffer.charAt(startLastMsg))
startLastMsg++;String msg = buffer.substring(startLastMsg, end);
foundMsg(msg);
startLastMsg = end;}
insideRootTag =false;}elseif(ch =='/')
status = XMLLightweightParser.VERIFY_CLOSE_TAG;}elseif(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;}elseif(ch =='/'&& head.length()>0){
status = XMLLightweightParser.VERIFY_CLOSE_TAG;
depth--;}
head.append(ch);}elseif(status == XMLLightweightParser.INIT){if(ch =='<'){
status = XMLLightweightParser.HEAD;
depth =1;}else
startLastMsg++;}elseif(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
*/publicString[] getMsgs(){String[] res =newString[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
protectedvoid setUp()throwsException{super.setUp();// Create parser
parser =new LightWeightXMLParser(CHARSET);// Crete byte buffer and append text
in = ByteBuffer.allocate(4096);}
publicvoid testHeader()throwsException{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
publicvoid testHeaderWithXMLVersion()throwsException{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]);}
publicvoid testCompleteStanzas()throwsException{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
publicvoid testIQ()throwsException{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
publicvoid testNestedElements()throwsException{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
publicvoid testIncompleteStanza()throwsException{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());}
publicvoid testCompletedStanza()throwsException{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
publicvoid testStanzaWithComments()throwsException{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]);}
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.
Today @Directi we had a few freshers from college come down for an interview. As part of the interview process, I was making a presentation to them about code smells. During this I was thrashing the whole “comments in code” 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 renderedthis.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.
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.
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:
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,
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.
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!
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.
