Signs of stupid Java code

[Joona I Palaste]

just taken from a project I am unfortunately working with :-((

if(text.charAt(i)=='0'||text.charAt(i)=='1'||text.charAt(i)=='2'||text.charAt(i)=='3'||text.charAt(i)=='4'||text.charAt(i)=='5'||text.charAt(i)=='6'||text.charAt(i)=='7'||text.charAt(i)=='8'||text.charAt(i)=='9'||text.charAt(i)=='-')
{}
else
{
return false;
}

Whoa! I hope you rewrote that as:

if ("0123456789-".indexOf(text.charAt(i)) < 0) {
return false;
}

or something equally more sensible than the original.

 

[Darryl L. Pierce]

just taken from a project I am unfortunately working with :-((

if(text.charAt(i)=='0'||text.charAt(i)=='1'||text.charAt(i)=='2'| text.charAt(i)=='3'||text.charAt(i)=='4'||text.charAt(i)=='5'|
text.charAt(i)=='6'||text.charAt(i)=='7'||text.charAt(i)=='8'|
text.charAt(i)=='9'||text.charAt(i)=='-')
{}
else
{
return false;
}

A junior engineer under me once wrote a patch of code to validate an email
address's format using similar code. I rewrote it using the following:

String address;

if(address != null && address.length() > 0 &&
(address.indexOf("@") > 0 && // it has the @
address.lastIndexOf(".") > address.indexOf("@"))) // there's a . after @
{
// the address is valid
}

He looped through each element of the email address and had flags to check
the position of the @ and for whether there was a . afterward, etc. Very
verbose and it kept *failing* on boundary conditions...

 

[brougham5]

if(address != null && address.length() > 0 &&
(address.indexOf("@") > 0 && // it has the @
address.lastIndexOf(".") > address.indexOf("@"))) // there's a . after @
{
// the address is valid
}

Hurm... Duplicate code. Even worse, taking a performance hit for
calculating the same value. Why are you calling indexOf("@") more than
once?

I wouldn't hold this up as a "good" example.

 

[Ben_]

What about using javax.mail.internet.InternetAddress instead ?

 

[Andrea Desole]

Actually I was thinking more something like

if ( text.charAt(i) < '0' || text.charAt(i) > '9' && text.charAt(i) != '-' )
return false;

(I'm just guessing that '-' is coming '9', but this can be easily
changed of course)

and I thought about changing it, but it's not code I'm working directly
with. And I saw some other masterpieces :-(
The big problem is not when you see a few lines like that, but when
there are things in the architecture (which is not documented) that take
days to fix, and turn into a mess whatever you do.
Basically I'm spending my time being very quick and extremely dirty.
Okay, sorry, I had to express my frustration ;-)

 

[Jesper Nordenberg]

A junior engineer under me once wrote a patch of code to validate an email
address's format using similar code. I rewrote it using the following:

String address;

if(address != null && address.length() > 0 &&
(address.indexOf("@") > 0 && // it has the @
address.lastIndexOf(".") > address.indexOf("@"))) // there's a . after @
{
// the address is valid
}

Too inefficient, I would use:

if (address != null) {
int index = address.indexOf('@');
if (index > 0 && address.indexOf('.', index + 1) >= 0) {
...
}
}

I guess there are different levels of programming stupidity (no offense ;-).

/Jesper Nordenberg

 

[Michael Borgwardt]

Too inefficient, I would use:

if (address != null) {
int index = address.indexOf('@');
if (index > 0 && address.indexOf('.', index + 1) >= 0) {
...
}
}

I guess there are different levels of programming stupidity (no offense ;-).

Indeed there are. Your version is in fact the stupid one because it sacrifices
clarity for an almost certainly totally irrelevant and very tiny performance gain.

 

[Rene]

Indeed there are. Your version is in fact the stupid one because it
sacrifices clarity for an almost certainly totally irrelevant and very
tiny performance gain.

And both examples are wrong because "user@localhost" and "user@hostname"
are fully valid email addresses. They don't have a single dot, but still
are valid *and* reachable in the local net.

The second solution also seems to allow [email protected] which however
is NOT valid.

CU

Rene

 

[Thomas G. Marshall]

That tought occurred to me as well, but besides being a contrived
solution, doesn't that defeat the purpose of a singleton? Or should I
make the subclass a singleton as well?
I solved the problem by making getInstance() static.

If your getInstance() routine looks something like this:

public static Singleton getInstance()
{
if (self == null)
self = new Singleton();
return self;
}

Be warned that it isn't thread safe.

 

[Timo Kinnunen]

Indeed there are. Your version is in fact the stupid one because
it sacrifices clarity for an almost certainly totally irrelevant
and very tiny performance gain.

One three-line if-statement is clearer than two one-line if-statements?
Really?

 

[Michael Borgwardt]

One three-line if-statement is clearer than two one-line if-statements?
Really?

The two version have the *same* number of code lines.
What makes Jesper's version less clear is the use of the temporary variable,
especially in the two-parameter indexOf() call - exactly the things he promoted
as an improvement.

 

[Chris Smith]

If you go back to my original post on the subject of casting to Object, it
was related to casting the key for a Hastable to Object. I work with an
engineer who does exactly that pathologically. Even after telling him it's
unnecessary, he still does it.

I'd agree that this is "stupid Java code", but specifically because
there's actually a real purpose for such upcasts in resolving ambiguity
in arguments. No silly and pointless practice annoys me more than when
it suggests that there's something tricky going on, only to later reveal
on closer inspection that someone was just being dumb.

As for having to cast null to be an object, that would sound like the class
you're invoking should have a no-arg version of foo(), rather than you
having to cast null in order to invoke a method.

Perhaps. It really will depend on the situation.

--
www.designacourse.com
The Easiest Way to Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Chris Smith]

Not quite. ALMOST pointless, but not "utterly pointless".

Recall that synchronization in Java involves two different things:
1) mutual exclusion
2) memory barriers

I was wondering if someone was going to mention this... Really, though,
you have to be far smarter than I to *consistently* do the kind of
reasoning necessary to ensure that your program is correct when using
synchronization in this way. So I'd still be tempted to call this
stupid.

--
www.designacourse.com
The Easiest Way to Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Darryl L. Pierce]

Perhaps. It really will depend on the situation.

True, though I feel confident, again, in saying that if it's necessary to
specifically cast a null object then that would indicate to me that a
no-arg version of the method is required.

 

[Michael Borgwardt]

True, though I feel confident, again, in saying that if it's necessary to
specifically cast a null object then that would indicate to me that a
no-arg version of the method is required.

What about multi-parameter methods:

foo(String a, String b, String c)
vs.
foo(String a, String b, Map c)

Now if you want to use the second method with null as its third parameter,
you have to use a cast.

 

[Chris Smith]

True, though I feel confident, again, in saying that if it's necessary to
specifically cast a null object then that would indicate to me that a
no-arg version of the method is required.

I'll give a specific example, then, of a time when I decided (must have
been a few years ago) to design an API where casting null was required.
The interface (obfuscated a little because I know the prior client
wouldn't want the real code with identifying content made public) looked
something like this:

class Gadget { ... }
class FooGadget extends Gadget { ... }
class BarGadget extends Gadget { ... }
class BazGadget extends Gadget { ... }

class GadgetContainer
{
public void add(FooGadget g) { ... }
public void add(BarGadget g) { ... }
public void add(BazGadget g) { ... }
}

The three methods could also be called with a casted 'null' reference,
which actually had meaning in this context... it meant that although no
specific such gadget was being added, that we wanted to know that there
was such a type of gadget added that nevertheless wasn't being tracked
by the system. So we'd do a 'container.add((FooGadget) null)'.

Sure, there were other ways to write that. That one seemed to make
sense at the time. Even if it's not an ideal solution, it seems a far
cry from the kind of silly, pointless code that's otherwise been
discussed (such as your example of upcasting parameters to HashMap.put).

--
www.designacourse.com
The Easiest Way to Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Doug Pardee]

I was wondering if someone was going to mention this... Really, though,
you have to be far smarter than I to *consistently* do the kind of
reasoning necessary to ensure that your program is correct when using
synchronization in this way.

Agreed. I'd only consider using this in a situation where a single
thread is doing the writing and where the mutex-caused delays were a
known problem. I'd probably spend a whole day reviewing the
memory-sharing plan. And it would require a LOT of conspicuous
comments marking off hazardous areas for future modifications. This IS
a seriously spooky technique.

Besides, I wouldn't synchronize on "new Object()" anyway; why create
an object each time through?

So I'd still be tempted to call this stupid.

Yeah, it's still stupid code. But not necessarily pointless code.

 

[Darryl L. Pierce]

Chris Smith wrote:

Sure, there were other ways to write that.

And would those other ways have been a better design? Perhaps another method
that tood an instance of Class for that represented the type of Gadget
being registered so as to avoid the casting of null?

That one seemed to make
sense at the time. Even if it's not an ideal solution, it seems a far
cry from the kind of silly, pointless code that's otherwise been
discussed (such as your example of upcasting parameters to HashMap.put).

Maybe not as silly, but not what I would consider (and here we venture into
the realm of subjectivity) a good design. Being an architect, I would
reject such a design more likely than to accept it into the code base.

 

[Darryl L. Pierce]

What about multi-parameter methods:

foo(String a, String b, String c)
vs.
foo(String a, String b, Map c)

Now if you want to use the second method with null as its third parameter,
you have to use a cast.

There's too little information to make a determination here. Do both
versions of foo do the same thing? If so, and if it's acceptable to have
neither a String more a Map, then I again say that a 2-arg overloaded
version is required, or else a foo that specifically is for a
foo-without-a-string (since it's no longer overloading that version) or the
same for the map version for the same reason.

 

[Chris Smith]

Chris Smith wrote:



And would those other ways have been a better design? Perhaps another method
that tood an instance of Class for that represented the type of Gadget
being registered so as to avoid the casting of null?

That one, I definitely would have rejected out-of-hand, actually. It
strikes me as very ugly. In the end, I don't look at casting null as
something to avoid, as it seems you do. I do, in the final analysis,
think that have a addUntrackedFoo, addUntrackedBar, etc, in addition to
the overloaded add method might have made sense, with the general code
refactored to a private method... but that isn't how it got designed at
the time.

--
www.designacourse.com
The Easiest Way to Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation