`
| |
 |
 |
| Recent Thoughts
| Tags
|
|
|
|
Saturday, August 6th, 2011
Consider the following code to retrieve a user’s profile:
public bool GetUserProfile(string userName, out Profile userProfile, out string msg)
{
msg = string.Empty;
userProfile = null;
if (some_validations_here))
{
msg = string.Format("Insufficient data to get Profile for username: {0}.", userName);
return false;
}
IList<User> users = // retrieve from database
if (users.Count() > 1)
{
msg = string.Format("Username {0} has {1} Profiles", userName, users.Count());
return false;
}
if (users.Count() == 0)
{
userProfile = Profiles.Guest;
}
else
{
userProfile = users.Get(0).Profile;
}
return true;
}
Notice the bool return value and the use of out parameters. This code is heavily influenced by COM & C Programming. We don’t operate under the same constraints these days.
If we were to write a test for this method, what would it look like?
[TestClass]
public class ProfileControllerTest
{
private string msg;
private Profile userProfile;
//create a fakeDB
private ProfileController controller = new ProfileController(fakeDB);
private const string UserName = "Naresh.Jain";
[TestMethod]
public void ValidUserNameIsRequiredToGetProfile()
{
var emptyUserName = "";
Assert.IsFalse(controller.GetUserProfile(emptyUserName, out userProfile, out msg));
Assert.IsNull(userProfile);
Assert.AreEqual("Insufficient data to get Profile for username: " + UserName + ".", msg);
}
[TestMethod]
public void UsersCannotHaveMultipleProfiles()
{
//fake DB returns 2 records
Assert.IsFalse(controller.GetUserProfile(UserName, out userProfile, out msg));
Assert.IsNull(userProfile);
Assert.AreEqual("Username "+ UserName +" has 2 Profiles.", msg);
}
[TestMethod]
public void ProfileDefaultedToGuestWhenNoRecordsAreFound()
{
//fake DB does not return any records
Assert.IsTrue(controller.GetUserProfile(UserName, out userProfile, out msg));
Assert.AreEqual(Profiles.Guest, userProfile);
Assert.IsNull(msg);
}
[TestMethod]
public void MatchingProfileIsRetrievedForValidUserName()
{
//fake DB returns valid tester
Assert.IsTrue(controller.GetUserProfile(UserName, out userProfile, out msg));
Assert.AreEqual(Profiles.Tester, userProfile);
Assert.IsNull(msg);
}
}
This code really stinks.
What problems do you see with this approach?
- Code like this lacks encapsulation. All the out parameters could be encapsulated into an object.
- Encourages duplication in both client code and inside this method.
- The caller of this method needs to check the return value first. If its false then they need to get the msg and do the needful. Its very easy to ignore the failure conditions. (In fact with this very code we saw that happen in 4 out of 6 places.)
- Tests have to validate multiple things to ensure the code is functions correctly.
- Overall more difficult to understand
We can refactor this code as follows:
public Profile GetUserProfile(string userName)
{
if (some_validations_here))
throw new Exception(string.Format("Insufficient data to get Profile for username: {0}.", userName));
IList<User> users = // retrieve from database
if (users.Count() > 1)
throw new Exception(string.Format(""Username {0} has {1} Profiles", userName, users.Count()));
if (users.Count() == 0) return Profiles.Guest;
return users.Get(0).Profile;
}
and Test code as:
[TestClass]
public class ProfileControllerTest
{
//create a fakeDB
private ProfileController controller = new ProfileController(fakeDB);
private const string UserName = "Naresh.Jain";
[TestMethod]
[ExpectedException(typeof(Exception), "Insufficient data to get Profile for username: .")]
public void ValidUserNameIsRequiredToGetProfile()
{
var emptyUserName = "";
controller.GetUserProfile(emptyUserName);
}
[TestMethod]
[ExpectedException(typeof(Exception), "Username "+ UserName +" has 2 Profiles.")]
public void UsersCannotHaveMultipleProfiles()
{
//fake DB returns 2 records
controller.GetUserProfile(UserName);
}
[TestMethod]
public void ProfileDefaultedToGuestWhenNoRecordsAreFound()
{
//fake DB does not return any records
Assert.AreEqual(Profiles.Guest, controller.GetUserProfile(UserName));
}
[TestMethod]
public void MatchingProfileIsRetrievedForValidUserName()
{
//fake DB returns valid tester
Assert.AreEqual(Profiles.Tester, controller.GetUserProfile(UserName));
}
}
See how simple the client code (tests are also client code) can be.
My heart sinks when I see the following code:
public bool GetDataFromConfig(out double[] i, out double[] x, out double[] y, out double[] z)...
public bool AdjustDataBasedOnCorrelation(double correlation, out double[] i, out double[] x, out double[] y, out double[] z)...
public bool Add(double[][] factor, out double[] i, out double[] x, out double[] y, out double[] z)...
I sincerely hope we can find a home (encapsulation) for all these orphans (i, x, y and z).
Posted in Agile, Code Smells, Design, Programming | 1 Comment »
Thursday, July 21st, 2011
How would you kill this duplication in a strongly typed, static language like Java?
private int calculateAveragePreviousPercentageComplete() {
int result = 0;
for (StudentActivityByAlbum activity : activities)
result += activity.getPreviousPercentageCompleted();
return result / activities.size();
}
private int calculateAverageCurrentPercentageComplete() {
int result = 0;
for (StudentActivityByAlbum activity : activities)
result += activity.getPercentageCompleted();
return result / activities.size();
}
private int calculateAverageProgressPercentage() {
int result = 0;
for (StudentActivityByAlbum activity : activities)
result += activity.getProgressPercentage();
return result / activities.size();
}
Here is my horrible solution:
private int calculateAveragePreviousPercentageComplete() {
return new Average(activities) {
public int value(StudentActivityByAlbum activity) {
return activity.getPreviousPercentageCompleted();
}
}.result;
}
private int calculateAverageCurrentPercentageComplete() {
return new Average(activities) {
public int value(StudentActivityByAlbum activity) {
return activity.getPercentageCompleted();
}
}.result;
}
private int calculateAverageProgressPercentage() {
return new Average(activities) {
public int value(StudentActivityByAlbum activity) {
return activity.getProgressPercentage();
}
}.result;
}
private static abstract class Average {
public int result;
public Average(List<StudentActivityByAlbum> activities) {
int total = 0;
for (StudentActivityByAlbum activity : activities)
total += value(activity);
result = total / activities.size();
}
protected abstract int value(StudentActivityByAlbum activity);
}
if this were Ruby
@activities.inject(0.0){ |total, activity| total + activity.previous_percentage_completed? } / @activities.size
@activities.inject(0.0){ |total, activity| total + activity.percentage_completed? } / @activities.size
@activities.inject(0.0){ |total, activity| total + activity.progress_percentage? } / @activities.size
or even something more kewler
average_of :previous_percentage_completed?
average_of :percentage_completed?
average_of :progress_percentage?
def average_of(message)
@activities.inject(0.0){ |total, activity| total + activity.send message } / @activities.size
end
Posted in Agile, Code Smells, Java, Programming, Programming Languages | 11 Comments »
Sunday, May 15th, 2011
Many people will argue that there is more badly written code than good code. And its important to write comments to avoid these situations. Therefore we should encourage (force) people to write comments.
IMHO they are absolutely right that today many project suffer from poorly written code without any (good) comments. However every team I know, that suffers from this problem, has always been told (forced) to write comments. In spite of the emphasis on writing comments it has not really helped them.
I usually ask:
By asking developers to write comments are we really addressing the root of the problem?
.i.e. developers don’t invest quality time to write self-documenting code; code that clearly communicates its intent and does not require the deodorant of comments.
May be its time to try something different?
I have seen this myself many times, when we emphasize & educate the team on how to write clean code and ask them to stop wasting time writing comments, the code starts to communicate lot better. Its lot more maintainable. Also we have found that writing automated tests is a great way to document your intent as well.
This is how I would explain the concept Comments Smell to a team:
Writing comments that explain “how” or “what” the code does, what it does, is evil IMHO. 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).
- If developers don’t invest time to write clear code, what is the guarantee that they will write clear comments?
- Is doing a mediocre job at both (comments and code) better than doing a great job at just Code?
- Will it actually be more productive to do both or just one?
Remember the biggest problem with comments it that they fall out-of-sync with code very soon. So its not just about the extra investment to write good comments, but also the investment to maintain them.
One has to think hard to write code that expresses intent rather than write some sloppy code with poor abstractions and get away (washing their hands off) by writing comments. Developers have to take responsibility for writing code that others can easily understand.
Having said that, there are times when “the why” (why we are doing something in the code, a particular way) is not apparent by just looking at the code. So if we don’t find a suitable way to communicate “the why” through code, comment is the fall back option.
Note that comments are a fall back option in “the why” case rather than a default option.
Posted in Agile, Code Smells, Programming, Training | 3 Comments »
Wednesday, February 16th, 2011
Technical Debt is any technical issues slowing down the project due to hasty (short-sighted) decisions made at an earlier point.
All of us make bad decisions, but not fixing them and just differing them really leads to bigger problems as these issues have a snowball effect.
Technical debt can be introduced at various levels:
- Code smells is the most obvious one,
- But things like lack of (or poor) automation,
- poor choice of tools,
- fragility in the development environment
- and so on
can also contribute to technical debt.
Posted in Agile, Design, legacy code, Programming, Testing | No Comments »
Wednesday, October 21st, 2009
Starting with the obvious forms of duplication like Cltr+C & Cltr+V pattern to more subtle forms of duplication:
Thanks to Corey Haines and the folks who participated in the Biggest Stinkers session @ the Simple Design and Testing Conference 2009. Most of this information was discussed during that session.
Posted in Design | No Comments »
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 »
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):
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 »
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<<<<< /> />]]>]]></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.
Posted in Design, Programming, Testing | No Comments »
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 »
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.
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 | 14 Comments »
|