Bad coding standards?

[steve747]

A coworker of mine has been criticised in a code review for a code similar to
that Iisted below because it 'uses an exception for code flow control'. I
agree with him that its good code because its a reasonable way to check if a
string is/is not an integer, but I wonder if anyone knows of some
authoritative reference to back this up?

....
int i;
try {
i = Integer.parseInt(astringvariable);
} catch (NumberFormatException nfe) {
System.out.prlintln("it isnt an integer");
}
.....

 

[Stefan Ram]

i = Integer.parseInt(astringvariable);

One might say, that it is indeed questionable to use
exceptions for such a determination, but that the blame is not
on the application programmer, but on the API designer for not
offering a method

package java.lang
class Integer
static boolean isInt( java.lang.CharSequence text )

Determines whether this text represents an int number
in a way appropriate for java.lang.Integer.parseInt.

Or, possibly, there is such a method, and I am just not aware
of it.

 

[Eric Sosman]

Stefan Ram wrote On 04/19/06 14:27,:

One might say, that it is indeed questionable to use
exceptions for such a determination, but that the blame is not
on the application programmer, but on the API designer for not
offering a method

package java.lang
class Integer
static boolean isInt( java.lang.CharSequence text )

Determines whether this text represents an int number
in a way appropriate for java.lang.Integer.parseInt.

Or, possibly, there is such a method, and I am just not aware
of it.

NumberFormat.parse() might be of some help.

 

[Roedy Green]

A coworker of mine has been criticised in a code review for a code similar to
that Iisted below because it 'uses an exception for code flow control'. I
agree with him that its good code because its a reasonable way to check if a
string is/is not an integer, but I wonder if anyone knows of some
authoritative reference to back this up?

Just what does he propose?

If you wrote your own test for correctness, it might not exactly match
Sun so you could STILL get an exception.

Further, you end up doing the test twice then.

 

[Fred Kleinschmidt]

A coworker of mine has been criticised in a code review for a code similar
to
that Iisted below because it 'uses an exception for code flow control'.
I
agree with him that its good code because its a reasonable way to check if
a
string is/is not an integer, but I wonder if anyone knows of some
authoritative reference to back this up?

...
int i;
try {
i = Integer.parseInt(astringvariable);
} catch (NumberFormatException nfe) {
System.out.prlintln("it isnt an integer");
}
....

Well... using an exception is, by definition, code flow control.
What does the critizer want him to do? Not use a try..catch block at all?
What does he think exception handling is for, anyway?

One could use NumberFormat.parse() in this case, but not all cases have
such a function that "substitutes" for throwing an exception.

If the above code is really what is there (typo notwithstanding), something
should be done in the catch phrase other than just printing a warning,
to ensure that the resxt of the code "knows" that a poroblem has occurred.

 

[Thomas Hawtin]

NumberFormat.parse() might be of some help.

[...]

I would prefer to use my own method to accomplish this sort of check.
Something like

private boolean isNumeric(String text){
boolean result = true;

for(int index = 0; index < text.length();index++){
if(!Character.isDigit(text.charAt(index)){
result = false;
break;
}
}

return result;
}

isNumeric("1000000000000000000000000000000000000000000")? isNumeric("")?

Tom Hawtin

 

[Tajonis]

NumberFormat.parse() might be of some help.

While this is a valid method it too will produce an Exception
(ParseException) in the event that the if the beginning of the
specified String cannot be parsed.

IMHO

I would prefer to use my own method to accomplish this sort of check.
Something like

<code>
private boolean isNumeric(String text){
boolean result = true;

for(int index = 0; index < text.length();index++){
if(!Character.isDigit(text.charAt(index)){
result = false;
break;
}
}

return result;
}
</code>

 

[James McGill]

What does he think exception handling is for, anyway?

There is an argument on the premise that exception handling can be a
very brutal and expensive method of flow control. I believe that's
right, as processing an Exception can cause the creation of a whole lot
of objects, and isn't there some reflection/introspection that goes on?
Certainly a worse bet than setting or checking a boolean.

 

[James McGill]

isNumeric("1000000000000000000000000000000000000000000")? isNumeric("")?

isNumeric("0x4A00")? isNumeric("-2.17828")? isNumeric("NaN")?

 

[EricF]

A coworker of mine has been criticised in a code review for a code similar to
that Iisted below because it 'uses an exception for code flow control'. I
agree with him that its good code because its a reasonable way to check if a
string is/is not an integer, but I wonder if anyone knows of some
authoritative reference to back this up?

....
int i;
try {
i = Integer.parseInt(astringvariable);
} catch (NumberFormatException nfe) {
System.out.prlintln("it isnt an integer");
}
.....

Using an exception for flow control is a bad idea in general. Given the Sun
api, I think your coworker's code is fine.

Eric

 

[chris brat]

Hi,

I think it becomes bad code (not taking performance issues into
account) when you build reams of logic into your exceptions and have
your logic based on your exceptions.

An example is :

Assume that the String variable possibleInteger is input from some
external agent and it could represent an integer value or a character
string.

try{
// test if the value received is an integer
Intereger.parseInt(possibleInteger);

// do all your 'integer' related processing
. . .
. . .

} catch (NumberFormatException){
// parse failed so do the 'String' related logic
. . .
. . .

}

I've seen examples where a new developer was relying on an exception to
be thrown for his logic (approx 30 lines of code) to execute - seemed a
bit dangerous to me.

Chris

 

[nospawn]

Hi Chris,

I've seen examples where a new developer was relying on an exception to
be thrown for his logic (approx 30 lines of code) to execute - seemed a
bit dangerous to me.

Actually the number of lines of code in the exception handler does not
mean anything and is definitely not "wrong". Theoretically exceptions
handlers should come in two different flavors:

1-. Cleanup: Leave the current object in an state as valid as
possible i.e. satisfying its invariant. Basically clean up the
mess as possible before escalating the issue to the calling client.

2-. Retry: Recover as gracefully as possible from a case (exceptional)
not covered in the specification. The retry can be as complicated
as it takes.

Though, anything done within an Exception handler is out of the method
specification and therefore out of the SRS see Robustness in "Object
Oriented Software Construction" by Bertrand Meyer.

HTH,
Regards,
Me

 

[chris brat]

Hi,

I am aware of the different ways that exceptions should be handled -
agree fully that it should be for cleanup or retry, but not business
logic.

I stated that I think building code to rely on an exception to be
thrown instead of correct checking (I apologise if I wasn't clear about
that specific point before) is bad code or leads to bad code. What
happens if the exception is no longer thrown due to a new library
version. Or a developer new to the project is not aware of this tries
to refactor the code by removing any unecessary exception catches and
breaks it.

The number of lines of code in an exception catch does matter - if your
exception handling becomes too complex or becomes ingrained in your
business logic then it becomes more difficult to manage.

Would you prefer to look at an catch block of 5 lines of code (i.e. log
error message, set state, return or continue) or 30 (if, then, else,
while, log, change state, retry, send email, kick off process, persist
data ) which should only ever actually be executed IF there is a
problem ?

I choose 5.

Regards,
Chris

 

[alexandre_paterson]

Hi there,

nospawn wrote:
....

Though, anything done within an Exception handler is out of
the method specification and therefore out of the SRS see
Robustness in "Object Oriented Software Construction" by
Bertrand Meyer.

That is true in Eiffel where exceptions are really, well,
exceptional.

Less so in Java where it's basically impossible to use the
basic APIs without dealing with a great many not so
exceptional exceptions (the OP's example is a classic
example where, as already pointed in this thread, a
state-testing method would have been preferable).

Java exceptions != Meyer / Eiffel exceptions

They're (sadly I would add) completely different beasts.

IMHO that's one area where Eiffel got it right while Java
didn't... but YMMV.

 

[peter.rooke.comp.groups]

There's a section in Effective Java, Item 39:Use exceptions only for
exceptional conditions.
Bloch, makes a case for using exceptions for exceptional conditions,
and not control flow.

 

[Chris Uppal]

isNumeric("0x4A00")? isNumeric("-2.17828")? isNumeric("NaN")?

And what about non-ASCII numeric digits ?

And is ',' acceptable (and what does it mean) ? And how about ' ' ?

What about '2.998e12' ?

Once you've got answers to all of these questions (and more), then you have a
precise spec of what "numeric" means for your application. If, for some
reason, the spec is actually "must be acceptable to Integer.parseInt()", then
I'd say the OP's example was correctly coded, even well coded (if the test was
encapsulated properly). If not, then the code was functionally wrong -- not
just badly /expressed/.

-- chris

 

[bugbear]

James McGill wrote:




And what about non-ASCII numeric digits ?

And is ',' acceptable (and what does it mean) ? And how about ' ' ?

What about '2.998e12' ?

Once you've got answers to all of these questions (and more), then you have a
precise spec of what "numeric" means for your application. If, for some
reason, the spec is actually "must be acceptable to Integer.parseInt()", then
I'd say the OP's example was correctly coded, even well coded (if the test was
encapsulated properly). If not, then the code was functionally wrong -- not
just badly /expressed/.

Err. Agreed in all respects.

BugBear

 

[Thomas Hawtin]

James McGill wrote:
[ Tom Hawtin wrote: ]

isNumeric("0x4A00")? isNumeric("-2.17828")? isNumeric("NaN")?

If, for some
reason, the spec is actually "must be acceptable to Integer.parseInt()", then
I'd say the OP's example was correctly coded, even well coded (if the test was
encapsulated properly). [...]

The code of the previous poster (Tajonis) accepts my two examples at the
top, but Integer.parseInt does not.

I'm not really trying to make a point about the particular code in
question. The point is that if you repeat yourself (or someone else),
you are probably going to be inconsistent. Inconsistent is far worse
than even a woolly spec, for instance.

Tom Hawtin

 

[James McGill]

The number of lines of code in an exception catch does matter

That's not the big problem, to my reckoning. The problem with throwing
and catching an exception is that the processing of that can and will go
all the way down to the classloader level, doing a monstrous process of
building the stack trace, reflecting it's ancestry, and that sort of
thing, which just is too ugly for me to want to even think about.

 

[Andy Dingley]

Well... using an exception is, by definition, code flow control.

Not at all! - at least by the standards of the "structured programing"
movement of 25 years ago (which is about as contemporary as most
managers have got).

The problem is that exceptions are worse than GOTO and they're almost
like Intercal's "COME FROM" statement. You really have no idea of where
the exception was raised, you simply end up somewhere by magic. So if
you are going to use them for flow-of-control, then encapsulate them and
be sure that there's only a small known area of code that could have
raised them.

As to the question of writing your own pre-validator instead, then this
is far worse than an exception. If you expect .parseInt() to be able to
make sense of its input (about the only useful definition of "this is a
valid integer") then you have to leave that decision in .parseInt()'s
hands. Writing your own hokey pile of regexes is certainly wrong. Now
catchign an exception isn't the nicest design ever, but .parseInt ()
doesn't give you any other sort of status flag to work with - so
exceptions it is!

As to the efficiency question, then who cares? You're looking at
exceptions (a slightly heavyweight jump) compared to parsing integers
from strings (famously tedious). The comparison is vastly unequal and
minor comparisons are irrelevant.