Summary
Is that code doing what you think it's doing? Are you sure?
Advertisement
I ran into some code the other day that looked something like this:
public void methodOne()
{
Bar aBar = Foo.getInstance().getBar();
if(aBar != null){
// Do Bar type stuff to aBar
}
}
public void methodTwo()
{
Bar aBar = Foo.getInstance().getBar();
// Do Bar stuff to aBar
}
Looking at it this way it's pretty obvious what's going one, but in the specific case I'm talking about methodTwo was maybe 40 or so lines away in the file. So if you didn't write the code it's pretty easy to miss this on a casual fly over. I was really surprised to find this since it seems to fly in the face of one of the first things I discovered when I was cutting my teeth; trust no one. The implementor of the above code trusted that the code would never return null. The fact that methodOne checks the return of getBar() and method two doesn't is probably explained by semantics of the system not visible in the snippet given. That is a different topic though.
It's probably hard to count the number of times I've been scratched, clawed or outright bitten by trusting code. This was true even when I was operating under classic (some might say antique) development processes where I was the only one mucking about in a chunk of code. Trusting yourself is almost always a bad thing too. In an agile environment code I wrote today can quickly be changed tomorrow by someone else, and without my knowledge.
For myself I've placed a fair amount of stock in tools and techniques used in the design by contract (DBC) process (is it a process?). So I try to check all of my inputs for reasonable values. I always evaluate, at least in my head, what it would mean if I got back an unexpected value from a method call and add mitigating code. Metaphorically I've found that treating code like I somtimes treat my kids works well. If you have kids this is probably a familiar conversation:
You:Did you eat all the cookies? Kid:Not me! Umm, I was dead at the time. You:Are you sure? Aren't those cookie crumbs around your mouth? Kid:Oh. Yah, I did eat the the cookies. I forgot.
If I run into a bug and find all the cookies gone, I at least know the first few questions to ask. Following the XP tenet of "if it works do it all the time", asking where my cookies are has proven valuable to me over the years and is probably worth doing for you to. Trust me.
I don't think you need to "trust" code. The getBar() method should document what its output is. BTW - Java is nice in that most methods get Javadoc'd. If the function is documented to never return null, then there's no trust involved. If it's not documented, then you should check for null.
It's not clear to me which of the code snippets is bad. I get the impression you think methodOne is the correct implementation, but I'm not so sure. Obviously it depends a great deal on the exact semantics of the methods. Personally, methodOne is the one that would scare me. If I get an unexpected null, I'm in trouble, because I don't know how to deal with it. Ignoring it is potentially much worse than letting an error occur. At least there's a good chance I'll find the error, and the error should be easy to fix. But if I catch the null and do the *wrong* thing with it, then I've got the kind of bug that drives a programmer crazy. Is ignoring the null the correct response? Maybe, I can't tell here.
It might be better to put in an assertion that aBar isn't null (which is little more than a sort of documentation), but out of context I would never do anything beyond that to methodTwo. (And since I do lots of programming out of context -- such is the reality of maintenance -- this sort of decision is important and common)
(1) Instead of having the possibility of returning null, change getBar() to return a "Null Object" (a instance of a class that does nothing when you call its methods) so callers never need to test for null. Google for the NullObject Pattern.
(2) Instead of calling getBar() and then calling methods on the result, do "extract method" and "move method" refactorings to put "Do Bar type stuff" into Foo (or Bar). (see Law of Demeter). The question of whether getBar() returns null is no longer something the callers care about, so your calls are like this:
<pre> public void methodOne() { Bar aBar = Foo.getInstance().methodOneBarTypeStuff( args ); }
public void methodTwo() { Bar aBar = Foo.getInstance().methodTwoBarTypeStuff( args ); } </pre>
Personally, I think the code fails to communicate it's intent. Can getBar() return null? How would you know unless there were some javadocs or comments. While I think javadocs and comments are useful, I hate depending on them to communicate intent. So, I'd adopt a simple convention:
The effectiveness of this convention can be seen in the following modification to the orgiginal snippet.
public void methodOne() { Bar aBar = Foo.getInstance().getBarIfPresent(); if(aBar != null){ // Do Bar type stuff to aBar } }
public void methodTwo() { Bar aBar = Foo.getInstance().getBarIfPresent(); // Do Bar stuff to aBar }
Now it is clear that methodTwo has a logic flaw.
Trusting code is a matter of ambiguity. When code does not express the author's intent, then you should not trust it. However, if the code expresses it's intent well, then trusting that intent is less risky, and can lead to less code clutter and code that is much easier to read and maintain.
Choosing good names is just one small aspect of writing clean code -- but it's a very imporant one. Too many developers ignore it and just assume that if there code works it's good enough.
I will labor over names, changing them again and again until I have names that clearly communicate my intent. The time I spend on choosing these names will save me, and others, many times over. And help us all to create modules with less code, less complication, and better structure.
> Can getBar() return null? How would you know > unless there were some javadocs or comments. While I > think javadocs and comments are useful, I hate depending > on them to communicate intent. > > Choosing good names is just one small aspect of writing > clean code -- but it's a very imporant one. > > I will labor over names, changing them again and again > until I have names that clearly communicate my intent. > I got tied up in knots this weekend over naming methods that all returned (almost) the same thing given different data, so I sympathise. (In the end working through the confusion gave me a deeper insight into what I was doing and, I hope, results in better final code.)
Nevertheless, I'm not altogether clear here. Method names no more impose conditions on the code than do Javadoc comments. Do you think that it is safer to trust a (compulsory) name (hoping that it is a correct concatenated description of the method's function), than to trust any (optional) javadoc comments. The truth is, without examining the source code, you still cannot tell - in this example - that the getBar() may or won't a null.
I'd say that the amount of trust that can be placed in the method name is not different to that you can put in the comments.
> public void methodTwo() > { > Bar aBar = Foo.getInstance().getBarIfPresent(); > // Do Bar stuff to aBar > } > [/code] > > Now it is clear that methodTwo has a logic flaw.
Why is that a logic flaw? I submit that it probably isn't. What should you do if null is returned? Skip some instructions? Is this safe - or expected? Probably the better thing to do is simply let the exception happen and let your caller sort it out. You don't have enough context otherwise.
I object to having to check for null all over the place simply to produce a half working system. But the stupid implementation of null in Java is another topic for another day.
> > Rule #1 is "comments lie". Self-documenting code is > the best comment. > > ...then the conclusion would be "Self-documenting code is > the best lie." > iow, "Self-documenting code, isn't" ;)
I usually code methods that should return something to return null only in case an error occurred that did not cause an Exception within the method. Anything else will at least yield an empty object (so a clean slate).
I say usually as sometimes I decide to throw an Exception instead, usually a RuntimeException, depending on the seriousness of the problem.
For example, a database wrapper should not return null if a ResultSet is empty (it should return an empty List of retrieved data or an empty DTO) but should throw an Exception if there was an error accessing the database (connection failed, invalid table or fieldnames, etc. This may include application-specific failures like a record that MUST be there else the database integrity is lost).
Previous Topic
