Why Methods Should Check Their Arguments
I’ve probably wasted two hours over the last couple of days trying to debug this line of code:
private static final QName name = new QName("valid-isbn", "http://www.example.org/books");
Do you see the bug? I’ve made it even easier to find by showing you just the line that contains the bug; but it still doesn’t jump out at you. Originally, of course, I didn’t know this was the buggy line. The exception was thrown somewhere completely different in the code base, but this is indeed the buggy line.
The problem is that the arguments are swapped. This line should have been
private static final QName name = new QName("http://www.example.org/books", "valid-isbn");
This is an example of poor API design. A method should not have two arguments of the same type that can be confused for each other if you can avoid it. If you can’t avoid it, then the method should check its arguments to make sure that the right one is in the right place; and throw an exception if it isn’t.
The QName class could easily have detected that I was constructing an object in an invalid state with an illegal local-name and illegal namespace URI. It should have noticed that and thrown an exception, in which case this bug would have taken 30 seconds to diagnose and another 30 seconds to fix. Instead the exception this caused didn’t happen till much later in the program, and it had a very unhelpful error message that wasn’t closely related to the real problem. Early, immediate, and obvious failure is vastly superior to delayed failure.
Interestingly this a case where even stronger typing would have helped, a lot; though doubtless the Smalltalk enthusiasts will explain to me exactly how this could never have possibly happened in their playpen; and if it did, they would have debugged it at runtime using a piece of chewing gum, a bobby pin, and a pocketknife they got out of a Crackerjack box.
FYI, XOM would have caught this error immediately and saved me hours of debugging time; but for the current project it’s necessary that I use DOM and JAXP 🙁
June 13th, 2006 at 9:57 am
I agree with your points though, and I’m an advocate of XOM too 🙂
June 13th, 2006 at 10:11 am
This article reminds me problem in XOM, where Element.getAttribute(String localname, String uri) doesn’t check whether its first argument is valid localName or not (http://lists.ibiblio.org/pipermail/xom-interest/2005-May/002266.html)
What do you think about checking arguments in getters?
June 13th, 2006 at 11:45 am
um, sounds like you want to write in Eiffel
June 13th, 2006 at 11:46 am
See:
http://www.cincomsmalltalk.com/blog/blogView?showComments=true&entry=3327649226
for my response.
June 13th, 2006 at 11:52 am
Erm.. well as a Smalltalker I *could* have debugged it at runtime and this would probably have led to the problem very quickly (without the need for gum and pins). However, I doubt such an error would ever have been coded in the first place given that the method would probably have been named #withUrl:isbn: and would have been called (incorrectly) as follows:
name := QName withUrl: ‘valid-isbn’ isbn: ‘http://www.example.org/books’.
Even my misty old eyes could spot this without recourse to the debugger.
June 13th, 2006 at 12:51 pm
Forget those Smalltalkers! Really, you should be using objective-c and its named variables (oops, lifted from Smalltalk… drat!). The method signature would look something like this:
– (String *) QNameGetIsbm:(String *) isbnString fromUrl:(String *)urlString
If you don’t like that, use something like the Ruby idiom where you pass parameters as elements of a hash:
@isbn = QName( {:isbn => isbnString, :url => urlString} )
works the same as…
@isbn = QName( {:url => urlString, :isbn => isbnString} )
The problem isn’t the API or strong-typing (how would you tell one (String *) from another without running each through a validator anyway?). Pick a language that helps self-document.
June 13th, 2006 at 2:43 pm
If you really want to buy in to the string typing argument the the constructor signature wouldn’t be:
public QName(String, String)it would be
public QName(URI, LocalName)of course, whether that is a good thing is open to debate.
June 13th, 2006 at 3:26 pm
Ah, yes, I see where strong typing comes into play.
Seems like this would lead to a proliferation of classes. More code = more maintenance = more bugs. I’m curious to know if this would lead to fewer bugs overall due to the magic of strong typing. Experience tells me that *more* code rarely leads to *fewer* bugs regardless of what tricks the compiler can perform.
No thanks. 🙂
Java has gotten so much syntactic sugar the last decade that it puzzles me why something like named arguments can’t make the cut. Code is so much more understandable with it. Granted, it does nothing to make the language more powerful but it does quite a bit in helping our brains comprehend intent.
June 14th, 2006 at 2:33 am
Good point !
June 14th, 2006 at 6:27 am
Interesting points about named arguments. If I ever get around to designing my own programming language, I’ll add them to the list of features to consider. However, in Java as it exists today, there are coding principles that would have avoided this bug. Unfortunately the
QNameclass didn’t use them.June 13th, 2006 at 11:00 am
I seriously considered checking arguments in XOM parameterized getters too. (A regular getter has no arguments and thus doesn’t need to check them.) My feeling there, though, was that there just weren’t quite enough problems to justify the added cost of doing the check in the getters. At least a bad getter argument doesn’t corrupt the object, whereas a bad setter or constructor argument does.
One thing I did consider and might still implement is lazy getter checking. That is, check the getter arguments only if the parameterized getter fails to find anything.
June 14th, 2006 at 8:22 pm
I agree with Ian, in that at least the URL argument should have been a URL object. The ISBN is fine as a String, unless there was some mechanism for doing a lookup. Alas, if you do that, you’d probably end up writing a convenience method that took a pair of Strings, turned them into the URL and ISBN objects, and then instantiated QName. And then you’d be back where you started.
So, fail early and fail often, especially in constructors.
;ted
p.s. Do you think you could turn off the full (left and right) text justification? It makes it harder to read than ragged-right text, especially with code that doesn’t line-break well and the lack of hyphenation.
June 15th, 2006 at 4:27 am
The URL object perhaps could have been a
java.net.URIobject or some custom class. Thejava.net.URLclass carries too much lookup baggage, though. It can’t represent an arbitrary absolute URI, just URLs for which Java has an available protocol handler.Also, it’s a side point; but it’s worth noting that the first argument is not an ISBN number. It’s an XML local name which happens to be the string “valid-isbn”. You couldn’t tell from the limited context I provided, but this is actually the name of an XPath extension function that checks ISBN numbers to see if they’re correct.
June 16th, 2006 at 5:14 am
Even if the QName class went to all the effort of lexically validating its arguments you could still find combinations of relative URLs and local-names that would pass this validation in the wrong parameter position. So I don’t personally feel that the extra effort of validation is worth it, especially in a method that is called a *lot*. Sometimes you really just have to be familiar with an API, and it’s just unfortunate that the cost of that familiarity is hours spent debugging.
June 16th, 2006 at 5:44 am
Relative URLs are not correct in QNames. No infoset is defined for such documents. (The W3C spent a lot of time sorting this one out circa 2000.) Relative URLs should be rejected by this class, even in the right position.
June 19th, 2006 at 11:59 am
[…] [-[1]-] I just read this Why Methods Should Check Their Arguments by Elliotte Rusty Harold. I had an almost identical issue a few weeks ago. […]
June 23rd, 2006 at 5:41 pm
I have just been chasing a similiar problem although simpler problem in Node.query(String xpath) this afternoon.
I failed to start my query with // and it kept returning Nodes.size() = 0.
Previously, I had hardcoded my query String and had it working
and then switched to passing an argument which was mis-coded.
It would be nice if Node would throw an exception if it receives
faulty xpath syntax.
PS: Would you consider adding the sample
XOM/src/nu/xom/samples/XPathDriver.java
to http://www.xom.nu/samples.xhtml
Thanks
June 26th, 2006 at 8:03 pm
XOM does throw an exception if the XPath you pass in is syntactically incorrect. Your bug was that you used a legal XPath expression that happened to not be the XPath expression you really wanted. I don’t think XOM can read your mind to guess what you really meant to do. 🙂
November 4th, 2006 at 9:39 pm
Stronger typing won’t help (in all cases.) So you should validate your arguments. Which means you don’t even need strong typing.