Published by Fabian on 01 Nov 2009 at 11:13 pm
Why do people code like that? And how do you review it?
if (!""==paramString.trim() && paramString.trim().length() > 0 && paramString != null)
I do like to coach colleagues and totally stangers in writing good code. I believe there is also a lot I can learn.
My colleague Thomas tweeted:
unless(!disable_message_hiding) {...}quad negation. !@#$@# (via @mhevery)
I think I could have done this myself some time ago. You have to learn to avoid negations so this doesn’t pile up like that. I can understand that somebody might have coded that.
But coming back to the line at the beginning of this post:
if (!""==paramString.trim() && paramString.trim().length() > 0 && paramString != null)
How can somebody write this code? Obviously not test driven, because test would have showed that this code is actually mostly nonfunctional.
Ok, when beginning with Java it can happen to you that you check String equality with == and not with String.equals(). Interestingly this does work quite often (but people don’t know why).
trim() shows already some more advanced understanding, because why else would you understand that you want to remove spaces from the string first.
But after having understood that we need to trim() before comparing to empty string, why the additional length check?
And why the hell the not null check? I suspect there was once a stacktrace in the log and this was supposed to fix this. Obviously in reality this method almost never gets null.
So: How could this code have been created? I think the author had no understanding of what he was doing there.
if (paramString.length() > 0)
is code Newbies would write. Its not bad. it is actually covering most of the normal cases. It is almost not worse than the previous example but much easier to read.
Such “empty String” checking is almost in every code. This is why commons-lang provided us with something neat:
if (! StringUtils.isEmpty(paramString))
How do you handle talking about bad code in code reviews? Pair programming?
I found it hard. So i decided to go the way Douglas Crockford goes when he talks about JSlint:
Warning!
JSLint will hurt your feelings.
I say: “that code makes no sense”, “that code is stupid”, “that can be done way shorter”, “code repetition – thats evil”.
It is like the standard measure for code reviews:

WTFs per Minute
I usually try to avoid to call the author stupid. But I try also to be very clear on the code. Usually it helps showing how this can be done much better.
Can somebody recommend a paper or book on the topic?
Oh, in case I did review your code and say something nasty: I apologize. I want to make the code better. I want us all to produce better code. Better code saves us weekends!
Friedel on 04 Nov 2009 at 1:36 am #
LOL
sorry for trolling again.
I’m not allowed (or willing) to show some crazy WTF code we saw in our company.
One guideline in Code Reviews is, that we review the code and not our colleague – and we are used to see crazy code. Our code base has some really old modules which were tweaked over years. Some evil sins (C macro magic horror) have already been removed by newer, fancier implementations (which are sometimes questionable, too). Also, our working atmosphere is so good that we can blame each other as brainless idiots. We don’t do this! Really! But I believe that nobody in my team would be hurt if we do so.
Therefore my only recommendation is having a nice atmosphere.
Our problem is, that we often loose our focus on the code we are reviewing. We end up in discussions how to solve the problem in general and in a generic way. This is contra productive sometimes.
A footnote about tools:
I suppose JSLint is just the Java-incarnation of lint which we are using for ages. Also I’d like to notice that code writers in other languages (explicitly in C and C++) often try to use different compilers. Different compilers generate different warnings. However this is not as powerful as static code analysis. In my business this is already a requirement (see FDA Software Validation Guidance). I’d also like set a link to http://www.schneier.com/blog/archives/2009/05/software_proble.html
Friedel on 04 Nov 2009 at 1:38 am #
offtopic:
Am I your top commentator?
Andreas Ebbert-Karroum on 11 Nov 2009 at 1:01 am #
Also the null check in the end is very interesting — you will get a NullPointerException if that parameter really IS null, before that check is executed
I can only guess, that this code started with the not null check and then grew step by step with bugfixing efforts (without tests, just messing with the code, until the bug is gone)
What would be nice:
if (StringUtils.isNotEmpty(paramString))
… even more readable in my opinion.
Peter on 13 Apr 2010 at 10:26 am #
I think Pike and Kernighan’s “The Practice of Programming” is a great book to recommend to people who write this kind of code. (I even enjoyed reading it myself) Anything ever written by Pike is highly recommended
“The Pragmatic Programmer” is another example of a good book, but more for how to become a better programmer on the long term.
Frank on 11 Jul 2010 at 12:44 am #
I only know c++, I am self taught with no formal training. From what I can see in the code above … if (!”"==paramString.trim() && paramString.trim().length() > 0 && paramString != null) … The author is check to see if the string’s param is not null. and each of the 3 checks appears to be doing the same thing from a different approach. Im not sure if this is necessary, or not. I think I would of used a method/function instead.
The basic algorithm I would try to implement here would be on the lines of this…
Instead of using an if statement. I would use a function that returns a bool to whether or not the string is empty or not.
PROTOTYPE: bool StringLength( ParamN paramN, ParamN+1 paramN+1, … , ParamN+N paramN+N ) where the amount of parameters needed for each case would be passed in. The fewer the better.
Please reply about this comment. I could use advise myself.