I was feeling kind of shitty, it being Valentine’s Day and all, when I found this Java gem. The following Java code apparently accompanied a pug-fugly “Extreme GUI Makeover” of a simple email client at JavaOne 2005. This is how the Swing team codes.
while (!buffer.atEnd()) {
if (buffer.get() == '\\r') {
if (!buffer.atEnd() && buffer.get() == '\\n') {
if (!buffer.atEnd() && buffer.get() == 'F') {
if (!buffer.atEnd() && buffer.get() == 'r') {
if (!buffer.atEnd() && buffer.get() == 'o') {
if (!buffer.atEnd() && buffer.get() == 'm') {
if (!buffer.atEnd() && buffer.get() == ' ') {
return true;
}
}
}
}
}
}
...
Haven’t these people ever heard of loops? Thank god the header they were looking at was “From” and not “I-Am-The-Very-Model-Of-A-Modern-Major-General” or something, huh?
I think these people wrote their own email header parsing library when JavaMail has been around for years. That makes it double-Daily WTF material because of the awesome NIH factor. But JavaMail is a Sun product (so it actually was “invented here” in this case) and Sun employees apparently don’t know about it.
I’m speechless.
This code does not look for an email header. It looks for the start of a message in an mbox file. Besides, I am not sure using a loop would result in cleaner, easier to understand, code. What do you propose, Mr. Feeling-Kind-Of-Shitty Man?
Comment by boa13 — February 14, 2007 @ 5:44 am |
Your blog entry is not very fair. :
“It has just the amount of functionality we needed for the demo, that’s it.”
“Scott said it clearly enough: this is demo and we took shortcuts to be ready in time for JavaOne”
“We didn’t have the time to craft a real mail client, or the time to come up to speed on one of the many open source mail clients out there.”
This code snippet is ugly but when you’re on a rush and it’s just a demo that’ll be thrown away later, I can understand. I sure hope the author of that snippet doesn’t write the swing library like that
Comment by Monkeyget — February 14, 2007 @ 5:48 am |
I think writing your own code to muck about with mbox files when quality libraries like mstor already exist is a pretty bad idea, myself.
Comment by metacircular — February 14, 2007 @ 5:50 am |
Monkeyget: I know it was throwaway code when first implemented. However, it’s been over a year since JavaOne 2005 and they could have cleaned up the code.
The reason they didn’t have time to come up with a real mail client is because they didn’t take 20 minutes to Google and find out that a significant fraction of the drudgery involved in grokking SMTP has already been taken care of for them.
It’s just typical to me of how Java coders really have no concern for brevity or quality, readable code. They seem to have this attitude that code is write/autogenerate-once, read-never. I think that should change. If the Java community wants to change in response to Ruby and Rails, they could start by pining for beautiful code that reads like a Hemingway novel.
Comment by metacircular — February 14, 2007 @ 5:59 am |
As far as I know, JavaMail is not included with Java. That makes it kinda hard for the Swing team to use it in internal code.
The code does indeed look awfully repetitive, but I thought for a moment, and without allocating Strings/StringBuffers/StringBuilders, I think that the code I’d refactor it to might actually be harder to understand.
What would you refactor it to?
Comment by Ricky Clarkson — February 14, 2007 @ 6:27 am |
Correction, I re-read, it’s not internal code, but still, JavaMail would be an extra dependency, which unfortunately still seems awkward.
Comment by Ricky Clarkson — February 14, 2007 @ 6:28 am |
Um. I don’t know much about Java I/O, but something like (in Scala) “\r\nFrom “.map(c => if(!buf.atEnd && buf.get == c) true else false).filter(true!=).length != 0 would perhaps be the right way to think about it.
Hm, I think after talking smack about Sun employees and Java I’m going to have to actually defend my claims. This could be problematic.
Comment by metacircular — February 14, 2007 @ 6:37 am |
Pugs aren’t ugly, jackass.
Comment by Viper — February 14, 2007 @ 8:30 am |
Re: NIH, there’s an old post on thedailywtf regarding IHBIHBLRIA: It Has Been Invented Here But Let’s Re-invent It Anyway. My soul cries tears of boiling pitch that I’ve had cause to think of it more than once as an accurate descriptor over the past nine months I’ve spent at my current employer.
Comment by anontexan — February 14, 2007 @ 8:34 am |
to make it a little simpler, reverse the sense of the test to
IF ‘at end’ OR ‘doesn’t match’ test fails
to make it look something like the one liners the functional folks like, do
for(int i=0;i
Comment by dmh2000 — February 14, 2007 @ 8:43 am |
to make it a little simpler, reverse the sense of the test to
IF ‘at end’ OR ‘doesn’t match’ test fails
to make it look something like the one liners the functional folks like, do
for(int i=0;i < “\r\nFrom “.length();++i) if (buffer.atEnd() || (buffer.get() != “\r\nFrom “.charAt(i))) return false;
return true;
if you aren’t so concerned with compressing everything into something obtuse, do
String s =”\r\nFrom “;
for(int i=0;i < s.length();++i) {
if (buffer.atEnd() || (buffer.get() != s.charAt(i))) return false;
}
return true;
unfortunately you can’t iterate over a string using the simplified Java for-loop that or you could say
for(c : “\r\nFrom “) if (buffer.atEnd() || (buffer.get() != c)) return false;
return true;
if you are dead set on one exit point from the code then you have to change the return false to set a variable, break the loop and return it at the end.
Comment by dmh2000 — February 14, 2007 @ 8:46 am |
I don’t know Scala, but you might be able to rewrite that as…
…getting rid of the superfluous “if”, the “.filter()”, and the “.length = 0″ (which normally should probably be written as isEmpty, so as not to needless traverse a very long list). But I don’t think that is modeling the original problem correctly, since the “while” loop will suck up characters until we hit “\r\nFrom “.
Comment by Greg Buchholz — February 14, 2007 @ 8:47 am |
Create an array/list with the characters, and then iterate through it, getting from the buffer and comparing against the char in the array.
Here it is in Ruby (written with Java idioms in mind) cause I haven’t done Java in a long while and will muck up the syntax, translate to Java in your head.
def foundFrom(buffer)
chars = ["\r","\n","F","r","o","m"," "]
for char in chars
if buffer.atEnd() or buffer.get() != char
return false
end
end
return true
end
while (!buffer.atEnd())
return true if foundFrom(buffer)
end
Comment by Mike — February 14, 2007 @ 9:27 am |
Well, this thing is basically just a DFA right? So, I wouldn’t find this code that hideous if it were spit out by flex or jflex or whatever the java version of flex is called. However, as code written by a living, thinking human it is pretty gross. Schedule pressure is want to reduce the quality of one’s output though, so I think the initial writing of this code should be forgiven. That it is being pushed as a didactic tool some time later without having it refactored does reflect poorly, but most likely it reflects poorly on some non-programmers involved in the whole process. I believe this would get this job, and most of the others like it done:
public boolean stringMatch(LongAssJavaBufferType buffer, String, toBeMatched){
while( !buffer.atEnd(){
if(buffer.get() == toBeMatched.charAt(0)){
for( int i = 1; i
Comment by vegas — February 14, 2007 @ 9:30 am |
ack, stupid tab space hiccup:
public boolean stringMatch(LongAssJavaBufferType buffer, String, toBeMatched){
boolean matched = false;
while( !buffer.atEnd(){
if(buffer.get() == toBeMatched.charAt(0)){
for( int i = 1; i
apologies if doesn’t work to preserve my pretty printing
However, it should be noted that both the original code, and this function will both totally miss(I believe) the string ‘\r\r\nFrom’ because they don’t do anything to save characters from the buffer for re-inspection by earlier stages of the automaton(which I guess would make it an NFA, but they’re equivalent anyways…)
Comment by vegas — February 14, 2007 @ 9:44 am |
Clearly WordPress doesn’t like code very much. I have this code in a much more readable format over at my rather low-tech blog:
http://euler.cs.umb.edu/~vegas/blog.html.
Comment by vegas — February 14, 2007 @ 9:56 am |
WordPress ate my code, it’s here: http://euler.cs.umb.edu/~vegas/blog.html
Comment by vegas — February 14, 2007 @ 10:30 am |
dmh2000, your code doesn’t work like it should. Your code just tests if the buffer has \r\nFrom at the current position. It doesn’t actually search through the buffer until it finds (or doesn’t find) \r\nFrom .
Granted, the blog entry’s code is cut so the intent of the real code isn’t clear. The while loop at the start gives an indication. The real code has buffer.rewind(1); after the shown block of code to rewind and try for \r again where char didn’t match what was expected.
Comment by nixnax — February 15, 2007 @ 1:50 am |
nixnax, i just reimplemented the nested if stuff shown in the blog. you could take what i wrote, make it a subroutine, wrap it in the while loop with the rewind they want. but that is still horrible design. What they want is a lexical scanner, something that is well understood how to do, is done with a finite state automaton, either hard coded or automatically generated, is not done with nested if statements, and which won’t need any rewinds.
Comment by dmh2000 — February 15, 2007 @ 9:28 am |
8iThank’s for greate post.6t I compleatly disagree with last post . zjg
паркет 1v
Comment by ламинат — August 24, 2008 @ 11:28 am |