I recently came upon some code filed with the following
} catch (FooException e) {
//do something
} catch (BarException e) {
//do something
} catch (Exception e) {
throw e;
}
which is easily re-written as
} catch (FooException e) {
//do something
} catch (BarException e) {
//do something
}
and
if (!(flag == false))
which is easily rewritten as
if (flag)
What are some of the most common junk code examples and the easier/clearer/less verbose replacement code.
http://thedailywtf.com/ has loads of examples of bad code like this.
if (x == 1)
return true;
else
return false;
Rewrite:
return (x == 1);
Your examples do different things.
} catch (Exception e) {
throw e;
}
Will discard the original exception, create a new exception in the context of the currently-executing method, and throw that. The original exception will be lost. This is rarely what you want to do, your re-written example is better.
Ternary operator
dim str
if(predicate)
str = "a"
else
str = "b"
end
Rewrite
String str = iff(predicate, "a", "b")
Initialization with value when in case of null a default is used instead.
Foo x = null;
if(something != null) {
x = something;
} else {
x = default;
}
=>
Foo x = something != null ? something : default;
In languages where if is an experssion:
Foo x = if(something != null) something else default;
(bColour)?bColour=false:bColour=true; //could have done (i%2!=0)?bColour=true:bColour=false;
instead of:
bColour = !bColour; //could have done bColour=(i%2!=0);
Yes, the comment on the original mess was actually in the code, so I corrected it too.
Old-style C string manipulation seems to be a bugaboo for a lot of people.
I've got two right here in my emacs buffer. Example 1:
strcpy(label, "\0");
strcpy(comm1, "\0"); //!!!20060818 Added protective code for missing nulls
strcpy(comm2, "\0");
strcpy(dir, "\0");
This is basically calling a routine with all the associated overhead just to do:
label[0] = '\0';
Here's an even more fancy variant later on in the code:
strncpy(inq.szComment, "\0", 1);
Again, this is just a really elaborate and slow way to assign the nul character '\0' to the first element of inq.szComment.
Example 2:
// This is always good for string operations, but may not be necessary.
strcat(RpRecPrefix, "\0");
strcat(RpCircPrefix, "\0");
strcat(RpEventPrefix, "\0");
strcat(RpFdk, "\0");
strcat(RpAic, "\0");
strcat(RpDcl, "\0");
strcat(RpVis, "\0");
strcat(RpSnd, "\0");
strcat(RpL3Fsi, "\0");
strcat(RpStartState, "\0");
strcat(RpContRun, "\0");
These find the null at the end of the first string parameter, and then do nothing. I can't tell if the author thought he was protecting against not having a terminating nul, or if he thought this would somehow tack an extra nul at the end. Either way, he was wrong.
OK. Here's my personal favorite.
if ( *pnFrequency == 1 ) {
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 1 Hz",
*pliWriteRate);
}
else if ( *pnFrequency == 2 ) {
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 2 Hz",
*pliWriteRate);
}
else if ( *pnFrequency == 4 ) {
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 4 Hz",
*pliWriteRate);
}
else if ( *pnFrequency == 10 ) {
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 10 Hz",
*pliWriteRate);
}
else if ( *pnFrequency == 20 ) {
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 20 Hz",
*pliWriteRate);
}
else if ( *pnFrequency == 40 ) {
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 40 Hz",
*pliWriteRate);
}
else if ( *pnFrequency == 60 ) {
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 60 Hz",
*pliWriteRate);
}
else { // above frequencies used for analysis
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec", *pliWriteRate);
}
Note that the value of *pnFrequency we check for in every branch is identical to the value hard-coded into its sprintf, and that is the only difference in any and every branch. The whole thing should have been:
sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at %d Hz", *pliWriteRate,
*pnFrequency);
I just simply can't imagine how this file got to be 12,000 lines long...
List<String> list = new ArrayList<String>();
list.add("Foo");
list.add("Bar");
=>
List<String> list = Arrays.asList("Foo", "Bar");
In which case list would be immutable. If you want to achieve the same with a mutable list, create a util class:
public CollectionUtils {
public static List<T> createMutableList(T ... elements) {
List<T> list = new ArrayList<T>();
for(T elem : elements) list.add(elem);
return list;
}
}
And then you can use it:
import static CollectionUtils.createMutableList;
List<String> list = createMutableList("Foo", "Bar");
Before Linq:
for each listItem1 in list1
for each listItem2 in list2
if listItem1.value1 = listItem2.value2
listSimilarItems.add(listItem1)
end
end
After Linq:
listSimilarItems = (from items in list1 where list1.value1 = list2.value2).tolist
On the web side of programming, something I frequently see is the use of CSS rules like the following:
<div class="red">Error message!</div>
With the red style defined as:
red { color: #f00; }
Well, that's stupid. And then when they decide that the error message should also have a yellow background you see this:
<div class="red yellowback">Error message 2.0!</div>
Or even worse:
<div class="yellowback">
<h1 class="red">Error message 3.0!</h1>
</div>
The proper way to use CSS is to construct your HTML in a semantically correct way first, and then identify each part with either 'id' or 'class' names, so that you can target each section and apply the appropriate rules. For example:
<div id="errormessages">Error message!</div>
Instead of a <div> you could make it a paragraph, so that it stand's out even without styles (e.g., when using a text browser):
<p id="errormessages">Error message!</p>
And the CSS rule:
#errormessages {
color: #f00;
background-color: #ff0;
}