 |
 | Home | Contact Us | FAQ | Search & Site Map | Link to Us |
 | Sign In | Join | Other 45 Sites in Network |
Java Forum / General / April 2006Bad coding standards?
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"); } .... > 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. Stefan Ram wrote On 04/19/06 14:27,: >>i = Integer.parseInt(astringvariable); > [quoted text clipped - 12 lines] > Or, possibly, there is such a method, and I am just not aware > of it. NumberFormat.parse() might be of some help.  SignatureEric.Sosman@sun.com > 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> >> 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; [quoted text clipped - 8 lines] > return result; > } isNumeric("1000000000000000000000000000000000000000000")? isNumeric("")? Tom Hawtin SignatureUnemployed English Java programmer http://jroller.com/page/tackline/ > isNumeric("1000000000000000000000000000000000000000000")? isNumeric("")? isNumeric("0x4A00")? isNumeric("-2.17828")? isNumeric("NaN")? > > isNumeric("1000000000000000000000000000000000000000000")? isNumeric("")? > > 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 >>>isNumeric("1000000000000000000000000000000000000000000")? isNumeric("")? >> [quoted text clipped - 12 lines] > encapsulated properly). If not, then the code was functionally wrong -- not > just badly /expressed/. Err. Agreed in all respects. BugBear > [ Tom Hawtin wrote: ] >>> isNumeric("1000000000000000000000000000000000000000000")? isNumeric("")? >> 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 SignatureUnemployed English Java programmer http://jroller.com/page/tackline/ >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. SignatureCanadian Mind Products, Roedy Green. http://mindprod.com Java custom programming, consulting and coaching. >A coworker of mine has been criticised in a code review for a code similar >to [quoted text clipped - 13 lines] > } > .... 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. SignatureFred L. Kleinschmidt Boeing Associate Technical Fellow Technical Architect, Software Reuse Project > 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. >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. >>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). A programmer should be aware of all possible paths of execution (control flow) in his code. Herb Sutter uses this C++ example: String EvaluateSalaryAndReturnName( Employee e ) { if( e.Title() == "CEO" || e.Salary() > 100000 ) { cout << e.First() << " " << e.Last() << " is overpaid" << endl; } return e.First() + " " + e.Last(); } to raise the question »how many execution paths could there be (...)?« The reader now might try to answer this, given the knowledge that functions might throw exceptions. OK, this is C++, not Java, but something like this could be stated for Java as well. The point is that the programmer should be able to prove or to guarantee correctness for every possible flow of control and that -- with exceptions -- it is more difficult to see all possible flows of control, because the exception control flow is somewhat hidden (in Java with Runtime exceptions more than with checked exceptions), while the structured control flow is visible as control structures. The article by Herb Sutter is http://www.gotw.ca/gotw/020.htm >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 [quoted text clipped - 10 lines] >} >..... 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 >Using an exception for flow control is a bad idea in general. Then, in Java, we also still have http://download.java.net/jdk6/docs/api/java/lang/Throwable.html which might be used to modify the control flow in the same manner, but - by its wording - is not an »exception«. 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 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 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 > 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. Hi there, ... > 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. 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. steve747 schreef: > 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 [quoted text clipped - 9 lines] > System.out.prlintln("it isnt an integer"); > } First, I hope I don't write something which is already been told. However, I think I read all messages. If you use the above code for verifying user input, there would be better solutions: You can use a InputVerifier (see JTextField) to ensure the user can only type correct data. The InputVerifier would probably rely on similar code to (catching exception and then return false), but it looks slightly better. If the above code is used in parsing a document (like an XML-document or databasefield) which is most likely already valid, I won't think there's anything wrong with it. From this point of view only the the System.out.println(..) is wrong. I would use nfe.printStackTrace(); printStackTrace() is an expensive method. This can be an advantage, this assures Exceptions only occur sporadic. Free MagazinesGet these publications absolutely FREE for up to 12 months. There are no hidden fees and no obligation. Simply choose a title, complete the application form and submit it. Read more ...
|
Reply to this Message
|
 Sign In
 Join
 My Latest Posts My Monitored Threads My Blog My Photo Gallery My Profile My Homepage Start New Thread Enable EMail Alerts Rate this Thread
|
©2008 Advenet LLC Privacy Policy - Terms of Use
This website includes both content owned or controlled by Advenet as well as content owned or controlled by third parties.