The Artima Developer Community
Sponsored Link

Weblogs Forum
Trust No One

10 replies on 1 page. Most recent reply: Mar 3, 2004 6:06 AM by Jeroen Wenting

Welcome Guest
  Sign In

Go back to the topic listing  Back to Topic List Click to reply to this topic  Reply to this Topic Click to search messages in this forum  Search Forum Click for a threaded view of the topic  Threaded View   
Previous Topic   Next Topic
Flat View: This topic has 10 replies on 1 page
Rick Kitts

Posts: 48
Nickname: rkitts
Registered: Jan, 2003

Trust No One (View in Weblogs)
Posted: Feb 3, 2004 9:19 AM
Reply to this message Reply
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.


Jordan Zimmerman

Posts: 23
Nickname: jordanz
Registered: Jul, 2003

Re: Trust No One Posted: Feb 3, 2004 12:52 PM
Reply to this message Reply
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.

Ian Bicking

Posts: 849
Nickname: ianb
Registered: Apr, 2003

Which is bad? Posted: Feb 3, 2004 3:17 PM
Reply to this message Reply
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)

Keith Ray

Posts: 658
Nickname: keithray
Registered: May, 2003

Re: Trust No One Posted: Feb 4, 2004 12:11 PM
Reply to this message Reply
Possible refactorings:

(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>

Robert C. Martin

Posts: 111
Nickname: unclebob
Registered: Apr, 2003

Re: Trust No One Posted: Feb 4, 2004 1:29 PM
Reply to this message Reply
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:


getBar(); //cannot return null.
getBarIfPresent(); //can return null.


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.

Jordan Zimmerman

Posts: 23
Nickname: jordanz
Registered: Jul, 2003

Re: Trust No One Posted: Feb 5, 2004 2:47 PM
Reply to this message Reply
getBarIfPresent();

Hey, I love this. This is a great idea. Rule #1 is "comments lie". Self-documenting code is the best comment. I will now steal this idea for myself ;)

Matt Gerrans

Posts: 1152
Nickname: matt
Registered: Feb, 2002

Re: Trust No One Posted: Feb 5, 2004 3:16 PM
Reply to this message Reply
> Rule #1 is "comments lie". Self-documenting code is the best comment.

...then the conclusion would be "Self-documenting code is the best lie."

:0 (Sorry, I don't know the emoticon for "tongue-in-cheek")

Vincent O'Sullivan

Posts: 724
Nickname: vincent
Registered: Nov, 2002

Re: Trust No One Posted: Feb 9, 2004 8:08 AM
Reply to this message Reply
> 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.

Vince.

John D. Mitchell

Posts: 244
Nickname: johnm
Registered: Apr, 2003

Re: Trust No One Posted: Feb 10, 2004 2:05 PM
Reply to this message Reply
> ...then the conclusion would be "Self-documenting code is the best lie."

:-)

> :0 (Sorry, I don't know the emoticon for "tongue-in-cheek")

That would be :-?.

Todd Blanchard

Posts: 316
Nickname: tblanchard
Registered: May, 2003

Re: Trust No One Posted: Feb 25, 2004 1:12 PM
Reply to this message Reply
> 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.

Jeroen Wenting

Posts: 88
Nickname: jwenting
Registered: Mar, 2004

Re: Trust No One Posted: Mar 3, 2004 6:06 AM
Reply to this message Reply
> > 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).

Flat View: This topic has 10 replies on 1 page
Topic: Why Do We Always Look Ahead? Previous Topic   Next Topic Topic: What's worse than no comments?


Sponsored Links



Google
  Web Artima.com   

Copyright © 1996-2014 Artima, Inc. All Rights Reserved. - Privacy Policy - Terms of Use - Advertise with Us