Good afternoon pop-pickers! It’s Christmas and that means we’re run out of ideas for interesting TV programs so we’re producing another Top List of Things Voted By You!
Todays program is all about the code reviews and the 10 most common comments. You can bear these in mind when writing your next chunk of code.
10. Embedded Literal Values
Straight in at number 10 we have literal values embedded in the code.
Embedding literal values in code is bad, but why is it bad? I can think of two main reasons – but I’m sure there are more.
- Potential to introduce multiple bugs
If you’re writing some kind of data access class, chances are you’ll refer to database column names. Embedding those names in your code means that if they change (or you make typo) you have to change them in multiple places.
If you have to fix some problem with those names you have to fix them in multple places leading to another potential bug if you forget to update one of them – or make more typos etc. - Values are meaningless
Consider the code sample;
int days = 5*52;
Now this is a noddy example and doesn’t hold up to close scrutiny, but nevermind. Firstly, “days” isn’t the most descriptive name. And what does 5 and 52 mean? Changing the snippet to;
int days = 5*numWeeksInYear;is better, but I now think there’s a bug in the system, ’cause it looks like it’s working out the number of days in a year, so that should be seven, not five, right?
First sub-point, I don’t know if it should be 5 or 7 because the name “days” doesn’t give me a clue, but ignoring that…
Second sub-point, using “workDaysInWeek” instead of the literal value ‘5′ tells me exactly what this line of code is trying to do. Something I didn’t know before.
In short, use named values not literals.
9. null Checking
Down 5 at number 9 we have lack of null checking
This one has dropped several points, it has been replaced by 5. Checked Exceptions, for reasons that I shall explain.
To practise defensive programming you should be checking that the values of things are what you expect them to be. That means that making sure a thing is not null before calling methods on it. This is going to at least reduce the number of potential exceptions that are going to get thrown and possibly translated into HTTP Error 500s.
The reason for this dropping a few places is that NullPointerException is at least a runtime exception which means we can use the framework to deal with it.
8. multiple return points
Up 1; we have methods with multiple return statements.
A point perhaps open to argument, but having multiple return statements in a single method make that method more difficult to read and understand. It a form of spaghetti code – and we got rid of goto for the same crime.
It can also make the code harder to debug. In an stack trace you won’t see which return statement was executed when a method ended which means that you have less of an idea what that method did and how it executed. And remember that a method might call other objects and do ’stuff’ to other ‘things’ you don’t know how much stuff happened because you don’t know what return was used.
It’s also dead-easy to write methods that only have one return statement and it doesn’t cost you anything in terms of performance. The following quote is applicable to most items in this list, it’s good here though.
Any fool can write code that a computer can understand. Good programmers write code that humans can understand.
Fowler, “Refactoring: Improving the Design of Existing Code”
7. Code headers
Holding stready at 7 we have missing, wrong or un-updated code headers.
Code headers are still the easiest, quickest and most readable way to identify how and why a change was made to the source. Particularly if you’re viewing the source in something other than Eclipse.
6. Readability
Another non-mover. Code that’s difficult to read.
This is a catch-all for some of the other items in this list. We’re all writing code that needs to be maintained and supported by all kinds of people. Which, by definition means; not just the original author.
So what does making code readable mean? It means checking each line, construct or method and seeing if it is simple. Is it obvious what it’s trying to do? Does it do it in the most simple way? Could someone else, who doesn’t have the same context and history with it as [the author], read it and immediately understand it without having to spend large swathes of time digesting it?
So to get around this make sure that your code is laid out nicely, that things line up and are in logical chunks. Teniary operators do execute faster than if-than-else statements but at the cost of readability. So if you’re going to use them make sure that they are simple to read and aren’t overloaded with lots of different conditions.
Increasingly, people seem to misinterpret complexity as sophistication, which is baffling – the incomprehensible should cause suspicion rather than admiration. Possibly this trend results from a mistaken belief that using a somewhat mysterious device confers an aura of power on the user.
Niklaus Wirth
Classes over 400 lines (roughly) are too long. Overly long classes are trying to do to much. Similarly, methods that are longer than the length of your screen are too long and trying to do to much. Lines that are longer than 80-100 characters are again too long. We’re all only human, after all, so split things up and make them easier to read and understand.
These simple rules make the code easier to read which translates to;
- Easier to understand
- Easier to review
- Easier to support
- Easier to debug
- Easier to enhance
5. (Checked) Exceptions
Climbing 3 places we have the use of checked exceptions.
If multiple return statement make the code a bit spaghetti like, then exceptions make the code look really spaghetti like. We don’t use goto because it makes the thread of execution jump around all over the place… just like exceptions do. How is it a good idea to have a method than can exit on any arbitary line?
Exceptions also require a lot of boilerplate code to use properly. throws makes the method signatures bloated, while try/catch statements artificially inflate the methods and interrupt the flow of solving the original business problem. Instead of reading and manipulating a DOM document, I have to write a bunch of code to handle SAX exceptions, premature ends of files, file not founds and all kinds of other stuff that just gets in the way of solving the problem. At least with runtime exceptions I can rely on the framework to handle them for me…
Exceptions are open to some serious abuse. They are not logic flow controllers. Exceptions are supposed to describe, er, exceptional things. And bad things at that. Yes your code will behave differently when an exception is thrown, but it should not rely on an exception being thrown before it will complete some business task.
Exceptions of any type are evil, but by contrast; runtime exceptions kick the puppies whereas checked exceptions tie them in sacks and drown them.
So please think of the puppies next time you want to use an exception. And to reply to the next person who tells me “Sun/Spring/Hibernate/Whizzy-Framework uses exceptions”…
Just because the standard provides a cliff in front of you, you are not necessarily required to jump off it.
Norman Diamond
4. Comments
Up 1, we have the old favourite; inapproriate code comments.
Good code is its own best documentation. As you’re about to add a comment, ask yourself, ‘How can I improve the code so that this comment isn’t needed?’ Improve the code and then document it to make it even clearer.
Steve McConnell – Code Complete
Why do we even need to talk about code comments? Developing (not Programming) software at it’s most basic is the practise of writing good comments along with your code. Again, it might make sense to you now, but it may not make sense to the next person. In fact it probably won’t even make sense to you in a few weeks time when you’ve moved onto the next problem.
Several times I have done a code review for someone and asked “why” they have done something. There are not enough numbers in the world to count the times the author(s) have responded with “Erm… Ahh… I think it was because… I can’t remember. Oh, yes I can! It was so that… no that’s not right.”
Again, it’s back to the unwritten non-functional requirement that your code must be maintainable and supportable by all the team. Comments help reach that goal.
3. Copy and Paste vs. Inheritence
Holding steady in third place is code that uses copy/paste code.
Every line of code any developer writes is potentially introducing a bug. So if we want to statistically reduce the number of potential bugs in the code base we should write less code.
A simple way to do this is to use inheritence. I shouldn’t have to explain what that means. But I shall explain it’s essence, it’s basically getting functionality for free from some other class. So if I have one class that is the parent of 10 other classes and shares it’s functionality with them, I only have to write that code once. Rather than write it once and then copy/paste it into the others.
The obvious thing here is that if we use cut/paste and there is a bug in the code, we have to first of all remember all the places we pasted the buggy code into and we have to remember to change them all. Of course, if our fix introduces a second and different bug we’ve got 10 additional changes to make.
That’s the alternative to just writing the code once, and then making the fix once. It also means that you only need one suite of unit tests for that functionality. Whereas, if you copy the code into more than one place you will fail your code review if you don’t have a unit test for every place that code has been copied to.
Inheritence makes using getters and setters within an object much more important. So using;
getMyAttribute();
rather than;
this.myAttribute;
Unfortunately that particular issue was at number 11 and therefore doesn’t feature in this list.
2. OO Design vs. Procedural Code
Down 1 we have the practise of writing procedural code in an Object Orientated environment.
Object Orientation is not the be-all and end-all of the programming world. It (probably) isn’t the greatest thing in the whole wide world. However, if you’ve chosen to be a Java and therefore OO shop and so you should be using OO principles in all our design and implementation.
That means modelling things as real objects with real data and real behaviours. It does not mean writing some big class that calls a bunch of procedural-based methods in a procedural-based way.
What else this means is that you should not be writing classes that consist soley of static methods. Here is a good article on why that’s bad (if you can ignore that fact it comes from the MSDN!) Inside Architecture: Are Helper Classes Evil?
Another good article about the lack of polymorphism of static-method-only-classes can be found here; Java forum thread
We can argue on the benefits and restrictions of OO verses other methods, and that’s fine. But at this point in time we have decided to be OO, so you should developing in that way.
1. Unit Tests
And our new Christmas Number 1 is unit test coverage.
For the purposes of this article I have extended the scope of “Unit Tests” to include all automatically executed tests, so that includes unit, integration and functionality tests.
How many times am I going to write about how important tests are? About how they save you time, how they make refactoring easier/quicker/safer? About how they help you understand if your code is working as intended? It seems at least once more…
Unit tests are important tests because they save you time, they make refactoring easier/quicker/safer, they help you understand that your code is working exactly as intended..
One more thing I will say about unit tests is this. If you’re going to do it then you should do it right. That means testing every possible situation, every possible permutation and every possible variable. There is to be none of this “Oh, I don’t need to test that since it’s an optional value.” It may be optional, but that implies that it’s going to have different values and you therefore need to make sure that your code behaves correctly for all possible permutations for all of those “optional values” or whatever the excuse may have been.
Justification
That’s about all we have time for this episode, so let’s wrap it up with one last word of justification.
I don’t think anyone can argue (with a straight face) that these things don’t make the code easier and cleaner to read. Yes, some of them might cost us a little bit of extra typing (e.g. using getters/setters rather than directly calling the attribute or using named values rather than literals), but that time is spent in the writing of the code something that we only do once. The saving comes from the reading, maintenance and bug fixing tasks we do – and we do plenty of those.
As Developers (not Programmers, there is a difference) one of our non-functional obligations is to make our code readable and maintainable to the next Developer, something might make complete sense to us but we all work in an environment where everyone has a different skill level so we need to make sure that our code is supportable by everyone and that means making it clear and readable.
It also means making it correct; something that the above list helps us do.
January 7, 2009 at 13:03 |
I have to strongly disagree on number 5 here, the checked exceptions problem.
You say “How is it a good idea to have a method than can exit on any arbitary line”. I say, if You are a developer, You write Your methods small, so You know where it exits (or You can step inside the method to see it for that matter). If You are the methods client (caller) You don’t care for what line did the method exit in. You do not care for the internal implementation of the method AT ALL. All You care of is to know why the method failed, and checked exceptions are an excellent way to provide feedback to the client.
You say that exception related code “just gets in the way of solving the problem”. Well tough luck, exceptions ARE the part of solving the problem. Exceptional situations (and how they are being reported) are a part of the method’s contract just like anything else. Deal with it. What’s more, I can’t think of any better way of reporting errors inside a method than checked exceptions.
You say that “At least with runtime exceptions I can rely on the framework to handle them for me…”. Really? So what kind of “handling” does the framework really provide (assuming that try and catch are left out as evil beacause they are “artificially inflate the methods and interrupt the flow of solving the original business problem”). Do You really consider a program crash with a generic error message a way of “handling” a problem?
You say “Exceptions of any type are evil”, I say that they are the sole thing that best distinguishes a good designer/programmer from a lousy one. Even a monkey can write code that implements some business logic assuming no problems are encountered during processing. The real art (or should I say engineering) is to write code that behaves sensibly in case of an error.
Best regards
Piotrek
January 7, 2009 at 15:02 |
Hi Piotr,
Thanks for your comments, I appreciate your candor and can see where you’re coming from. Let me explain a bit more why I dislike (checked) exceptions so much.
When I wrote this article I was working in a large team of people with very different levels of ability. Quality was often sacrified on the alter of “Just Get It Done” and sometimes because people just didn’t know any better. But that was the situation and the consequence of it was that the codebase was huge, unwieldy, confusing and of very poor quality. (Luckily, I work somewhere better now!)
You’re absolutely right when you say “You write Your methods small, so You know where it exits”. Of course we would never write long methods
, but the fact of my life (then at least) was the often people would write classes thousands of lines long, with methods hundreds of lines long. Sadly, such source files were not uncommon.
As a maintainer of (probably someone elses) code trying to figure out exactly where a method might exit given some problem is difficult given that kind of code. This makes debugging difficult. You’re right, I can run it up in the debugger and step through, but that pre-supposes an environment where the problem is easily reproducable. /Insert usual comment about unit tests here./
You’re right again that the caller of some method doesn’t care at what line the method exited, and I hope that I didn’t imply that it did. (This article is a bit old now and my memory is fuzzy.) I would say though that rather than accept that exceptional circumstances happen sometimes (and by definition, that should be rarely) I would write a method to be as considerate to the caller as possible.
Obviously, sometimes my methods are going to have to bomb-out because they can’t continue, but I would hope that when some bad things happen my methods would be able to cope well enough to prevent my caller from having to deal with whatever that bad thing is.
Exception catching code does get in the way of trying to solve a business problem. Consider the following canned example;
int someValue = -1;
try {
someValue = methodThatMightThrowException();
doSomethingWithIt(someValue);
} catch (CheckedExceptionTypeA ex) {
//handle
} catch (CheckedExceptionTypeB ex) {
//handle
} catch (CheckedExceptionTypeC ex) {
// handle
}
//do more stuff with someValue
Three lines of code just got turned into eleven – assuming I can handle each kind of exception on one line. And anything can happen inside those catch blocks.
I prefer runtime exceptions because they allow callers of the method to deal with the problem or defer handling the problem to something higher up the stack. Using the same example as before, I would prefer to be able to be be selective in what I want to handle and write;
int someValue = -1;
try {
someValue = methodThatMightThrowException();
doSomethingWithIt(someValue);
} catch (CheckedExceptionTypeB ex) {
//handle
}
//do more stuff with someValue
Where all the exceptions that the method might throw are actually runtime exceptions (forgive the confusing naming convention!
, but only CheckedExceptionTypeB is important to the caller. The other kinds of exceptions are not pertanent to the job at hand, or are more generic exceptions that more generic code should be able to handle. Obviously I can do this with checked exceptions also, but that means having to load my method header with extra throws clauses, which forces everything which uses my method to do the same (or catch them).
“Do You really consider a program crash with a generic error message a way of handling a problem?” Yes, but I would class it as a BAD way of handling the problem!
How can the framework handle exceptions? Well, if you’re lucky enough to be running in an AOP environment you can write exception handlers that get injected into the byte code around method calls, this means you can write the catch-blocks once and keep them all in one place. This obviously carries with it the usual trade-offs that AOP implies. Or you can design your code in such a way to to route calls through certain classes which catch exceptions. For example, introduce some factory or builder which will do some work for you, catch all the more standard and less specialized kind of exceptions in there and leave the callers of your methods to handle the specialised ones. But such things require real developers and not code-monkeys. 
There’s nothing (that I know of) in the Java language itself that will give me exactly the kind of framework exception handling that I would really want.
I would disagree with you on this; “they are the sole thing that best distinguishes a good designer/programmer from a lousy one”. A Monkey can easily write code that throws exceptions, that uses exceptions to control the flow of logic within a method, over-uses exceptions and throws them when they could possibly recover the situation inside the method. In my view and in some situations, throwing an exception is the lazy way out when the developer can’t think of anything better to do.
A good designer/programmer will, by all means, use exceptions where and when necessary. Like anything exceptions can be abused, and in my experience they are one of the easiest things to highly abuse. Writing code that uses checked exceptions liberally forces all users of the code to write lots of boiler-plate handling code which is often repeated in many places and it can be very difficult or impossible to refactor such duplication out. Also, given a large development team, the methods of handling those exceptions would probably not be done in a uniform way.
“(Checked) exceptions: Yes or No”, is very much a religious war that is oft faught on blogs and tech articles etc. You know where I stand, and it’s clear where you stand. I’m not convinced enough by your arguments to change my view significantly. My experience has taught me that some programmers will abuse them and that causes pain for everyone working on the code base. Having said that, your comments and my experience since I originally wrote this article, leads me to believe that I should instead be griping about “inappropriate use of exceptions” rather than “exceptions” in general – although being forced to always write a catch-blocks will probably always frustrate me. Also phrases such as “are evil” aren’t helpful and probably invoke to strong an emotion which doesn’t help discussion.
I sill maintain though, that if we were to be reviewing the same code together (or even each others!) the use of checked exceptions would be something I would pay particular attention to. I can be talked into situations where they are necessary, I do not advocate the removal of Exceptions in general, but I do believe that in some (possibly many) situations they can be changed to runtime exceptions or handled silently and internally within a method.
Thanks again for your comments.
Tom
January 7, 2009 at 15:56 |
Thank You for Your elaborated reply. I agree that exceptions in general are prone to abuse. I’m glad that You have separated the exception mechanism from the exception abuse in Your reply (a separation that, in my opinion, Your original post was lacking). Of course it is better to recover from the error “in place” than just throw an exception. But this requires a way to actually recover, and I have seen hundreds of methods where exceptions were caught immidiately after being thrown, and the error was “recovered from” by returning a meanigless value (most often null, which caused hard-to-track NullPointer/ReferenceExceptions somewhere lese in the code). I do not consider that to be recovering from an error or handling an exception and I would advise anyone to throw the exception instead. I hope there is an agreeement between us here.
However, I think we are missing one important point here. You consider writing throws declarations quite a significant overhead, But what is important, these declarations strongly encourage contract driven design (i think that this was the main reason checked exceptions were included in java). They hence allow to describe the method’s behaviour more completely: the method returns a significant and valid result or (xor) throw a defined checked exception. This way the contract is more clear to the callers of the method, a huge benefit. Typing some extra throws declarations seems to be little price for this. Note, that this is the main difference between unchecked and checked exceptions, the former being not a part of the contract and signaling a bug in the method (thus contract violation). So i cannot agree that “having to load the method header with throws declarations” should discourage anyone from enjoying the benefits of checked exceptions.
All the best
Piotrek
January 7, 2009 at 16:42 |
Hi Piotrek,
Sorry, I can’t leave it alone.
My gripes are multi-faceted. As you said, exception abuse is a bad thing and the abuse bit didn’t come across clearly enough in my original article. But, I still do take issue with having to deal with exceptions in and of themselves.
Returning null, sometimes, is a valid course of action. But you’re right returning that or “-1″ or whatever isn’t always helpful. If writing boiler-plate catch blocks irks me, then having to read (and re-read with every new revision) the Javadoc to see what return codes I can expect and handle all the different ones would be even worse!
Exceptions vs return-codes is another religious war I don’t want to get into!
If each type of exception describes a different kind of error case, that (possibly/probably) implies a different course of action my method should take. Does that imply that my method should start to do more than one job? Of course not because our methods are all small and do only one thing. But consider this;
buttonActionPerformed() calls readDataIntoStructure() calls openFile()
If “openFile” throws a FileNotFoundException (checked) then “readDataIntoStructure” must handle it maybe by asking the user for a different file name/path. But that’s not the purpose of “readDataIntoStructure” so it should instead re-throw that same exception up to “buttonActionPerformed”. Which is only a couple of lines of code, but make FNFE an unchecked exception and that boiler plate disappears from everything that isn’t interested in handling it.*
That’s why I prefer unchecked over checked.
Sadly, I’ve been less than clear again though. Writing extra throws clauses isn’t an arduous task. I can type fast enough and use my IDE/intellisence well enough that it doesn’t slow me down. But for every new exception I put in the throws clause someone has to write several lines of code to catch it. That’s what I don’t like.
Design by contract is a good thing, I like knowing what exceptions a method might throw – providing I have the option of not catching them if I want to. I did a stint writing some .Net code for a previous company, and that doesn’t enforce the throws clause (I think, I don’t know much about .Net) but I found it very confusing and difficult trying to work out what exceptions a method might throw.
Which is a problem with runtime exceptions over checked exceptions.
Take a look at the JavaDoc for java.io.File (1.5). Many of the methods descibed in that document declare that they thrown NullPointerException and IllegalArgumentException, both runtime exceptions. So I know the contract of those methods but I can chose to ignore the exceptions they throw if I want to. I’m happy to list the runtime methods my method might throw on the throws clause or better yet have the compiler add it for me based on some annotation or JavaDoc**.
But make it easy for me to see who is throwing what and give me the freedom to elect not to catch it if I don’t want to.
Checked exceptions make the “who” clear, but they lock me in to the strait-jacket of catching and handling/rethrowing; and I don’t like that.
Runtime exceptions make it more difficult to see who is doing what but give me the freedom to not catch the exception if I want to; and I don’t like that.
Give me the middle ground; the visibility of what might be thrown coupled with the ability to ignore the exception if it is and that would be perfect. But that is my opinion.
Thanks for this debate. It’s made me think a lot harder about what was originally an article steeped in emotion!
Tom
* Could my IDE be made clever enough to see that if “openFile” has a runtime exception in it’s throws clause, then put a visual warning next to everyline the calls “openFile” someone in it’s stack?
** Which I, as a good developer, will of course maintain…
January 9, 2009 at 13:48 |
Hello,
I’m confused now. You say:
“consider this;
buttonActionPerformed() calls readDataIntoStructure() calls openFile()
If “openFile” throws a FileNotFoundException (checked) then “readDataIntoStructure” must handle it maybe by asking the user for a different file name/path. But that’s not the purpose of “readDataIntoStructure” so it should instead re-throw that same exception up to “buttonActionPerformed”. Which is only a couple of lines of code, but make FNFE an unchecked exception and that boiler plate disappears from everything that isn’t interested in handling it.*
That’s why I prefer unchecked over checked”
First of all, while i definitely agree that readDataInfoStructure method is not the right place to handle the FNFE, I do not understand how will buttonActionPerformed know that readInfoStructure can throw this exception if You make it unchecked? Since the readInfoStructure method is non-transparent (black-box) from the buttonActionPerformed method’s point of view it is the readInfoStructure that throws this exception. How will You formally define the contract for behaviour when a file is not found problem(in the programming language terms) if You make this exception unchecked? readInfoStructure cannot just “not-care” for this exception. It throws it so it needs to signalize the possibility of an exception instance being actually thrown at runtime to the caller.
Secondly, You cannot just make an arbitrary exception unchecked. Wheter a particular exception is checked or not is not a matter of taste or programmers convinience. The main difference is that checked exceptions define events defined by contract and thus caller-manageable (probably caused by the callers error, such as not providing a valid file path). Unchecked exceptions on the other hand signal method implementation internal error. Actually throwing an unchecked exception is a contract violation and thus need not to be managed by the caller.
And third, I can’t really understand why do You consider handling (whether catching or declarating) exceptions such a drag. You repeatedly call the exception related code varoius pejorative names such as boilerplate and so on. It’s really not too big of an effort to write a throws or catch clause, Eclipse for example does this automatically with two mouse clicks. I can’t figure why’s all the buzz about exceptions. Nobody runs around saying that, for example, method parameter types (or an explicit parameter list declaration for that matter) is “boilerplate” code and complaining for having to “load his/her method header with it”. Just try to look at exception throws declarations like they were parameter declarations – a part of the contract (which, in fact, they are).
Best regards
Piotrek.
January 9, 2009 at 13:51 |
Sorry, I just noticed that I have written “declarating” instead of “declaring”, must have been some temporary brain malfunction
January 10, 2009 at 21:11 |
I think that “brain malfunction” is possibly my default state.
You’re confusion is very close to my frustration. There is not a way in Java to get the behaviour I would like.
I like your definitions of checked and unchecked exceptions. Are they from the Java spec itself? I can’t remember, it’s been a while since I flicked through it and don’t think I’ve actually ever read it cover-to-cover. (Maybe I should…) But as I suggested before, take a look at Java.io.File. Many of those methods explicitly declare (in the JavaDoc at least) that the method might throw an unchecked exception. I think it’d be great to be able to enforce that at the compiler level.*
I would like to have all exceptions that might be thrown in a method explicitly listed in some annotation or throws clause of the method itself; and have the compiler enforce this. Or failing that, have the compiler warn me that an exception might be thrown from inside a method if it detects that it has not been caught and allow this to be chained through subsequent method calls. Couple that with making all exceptions runtime and you’d have what – in my opinion – would be the perfect solution.
But like I say, Java doesn’t do that.
Reading that paragraph again, I think I prefer the compiler warning idea since it would keep the amount of text in a source file down (aiding readability) but would still allow IDEs the abilty to flag the warning in the source code in whatever method your chosen IDE would use for other such warnings.
I think you’re reading my gripe and wondering how you could do something like that given the mechanisms that the current versions of Java give you. My point is, that Java doesn’t give you the mechanisms to do it. That’s part of my problem with exceptions. And given these limitations (if, indeed, they are limitations in your opinion) I have often found that when reading other people’s exception-laced code better solutions are available if the developers would think about the problem a bit harder.
Now whether any two people agree that this kind of melding of checked and unchecked is a good thing or not is another matter. As I said, in my opinion I think it would make code cleaner since it would result in less boiler-plate code, which leads me to…
My definition of “boiler plate code” is something that I’m forced to write (by the compiler) that is at least one step removed from the problem I’m trying to solve. If I’m trying to implement a complex algorithm, that’s all I want to be thinking about. I don’t want to have to put a whole bunch of try-catchs in the midst of otherwise complicated code unless that algorithm requires me to. If some exceptional circumstance means my algorithm has to stop immediately, why not let it be a runtime exception and have the caller of my algorithm sort the problem out.
This is why in my methods I will always check the input parameters before I start using them and throw IllegalArgumentExceptions for any that are out of bounds. I think this is fairly standard and a recognised (good) way of going about things. I don’t put null checks and similar in the midst of an algorithm implementation for the same reason, it adds noise that can hide/confuse people as to what the algorithm is doing.
Try-catch blocks give you not only additional lines of code to read through, but also additional levels of indentation which can hide the structure of the algorithm you’re trying to implement (however complex that might be). Typing the words or having the IDE insert them for you isn’t the problem. Reading them is the problem. (Good) Developers spend far more time reading code than writing it, especially if you’re having to go back to some of your old code or other people’s.
If my memory serves, the default insert for Eclipse when it handles an exception is a comment saying “TODO”, I think that NetBeans will put in a logging call for the exception and/or a TODO comment also. That’s great, now your compiler will not complain at you, but I’ve seen developers do exactly this and not pay any attention to what to do now they’ve caught the exception. You mention in a previous comment how difficult it can be to track down the source of a rouge exception that was thrown. Contrast that with how difficult it is to track down a bug because someone left their catch block empty.
Of course you can wrap the whole internals of the method inside a big try-catch block, but I start thinking “code smell” when I see that. If it’s only a short method does it make more sense to just re-throw the exception, if it’s a long method are you happy catching the exception somewhere that might be “far away” from where it was thrown? The answers obviously depends on the specific code in question.
Naturally, these two points are drifting back to “exception abuse territory” and not “generic problems with exceptions”. But, I believe, if the mechanism I crave were to be available it makes exception handling that little bit more loose which actually makes it less prone to some forms of abuse. Obviously I don’t think that there will ever be a way to actually stop people using exceptions to control logic flow, but that’s not something I’ve figured out how to stop yet…
Actually, method parameter types are boiler-plate code. Maybe they are necessary with OO languages in general or just the type system that has been chosen for Java – I don’t actually know the answer to that one. But take a look at some Haskell code, the type system is a lot more clever than the Java one; in that it can infer the type for you (or it prompts you to specify it only when it cannot be inferred). Lots of the noise in the source files that us Java programmers have gotten used to just isn’t there in other languages and it’s amazing how much those other languages are (or at least, can be once you know the syntax) easier to read.
It’s a bit like listening to the radio. You can listen and hear fine, you can enjoy the music even when it’s not tuned 100% perfectly. But then go and listen to a CD recording on a high quality sound system and the difference you get in clarity is amazing. Obviously I’m not comparing my code to Mozart, but you get the idea.
Cheers.
Tom
* Of course, the problem then becomes, where do you draw the line? NullPointerException, things which every method might possibly thrown?
October 29, 2009 at 10:34 |
[...] “Top Ten Things I Gripe About In Code Reviews“: “8. multiple return [...]