The Artima Developer Community
Sponsored Link

Weblogs Forum
Offensive Coding

67 replies on 5 pages. Most recent reply: Sep 12, 2006 8:43 PM by Todd Blanchard

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 67 replies on 5 pages [ 1 2 3 4 5 | » ]
Michael Feathers

Posts: 448
Nickname: mfeathers
Registered: Jul, 2003

Offensive Coding (View in Weblogs)
Posted: Jul 14, 2006 1:56 PM
Reply to this message Reply
Summary
Tempted to code defensively? Maybe it's because you're dealing with offensive code.
Advertisement

Here's a little sample of code:

private void addResolution(List<Flag> flags) {
    if (flags != null) {
        if (supportedAdapters.contains(Adapter.LOW) 
                || supportedAdapters.contains(Adapter.MEDIUM)) {
            flags.add(new ResolutionFlag(ADAPTED));
        } 
    }
} 

What’s wrong with it?

No, it’s not the control flow. Sure, we could replace the conditional with a guard clause and get rid of some indentation, but the problem is more subtle than that. Most people are blind to it because it’s in their face every day.

The problem is this line:

if (flags != null)

Spurious null checks are a symptom of bad code.

That’s not to say that null checks are wrong. If a vendor gives you a library that can return null, you’re obliged to check for null. And, if people are passing null all over the place in your code, it makes sense to keep putting some null checks in, but, you know what? That just means that you’re dealing with bad code: you're dealing with code where people are actively making extra work for themselves and making code brittle in the process.

I’ve gotten more diplomatic over the years, but in one my early programming jobs, the guy who hired me called me over to his office and showed me a scheme that he’d created for our classes. Every method that we called could set a flag if it determined that it was passed a bad parameter. After a client called a method, it could check the flag and determine whether the call was successful or not. I said “Oh, cool, we can call it the ‘was I stupid flag!’” He gave me a withering look I’ll never forget, but he saw the point despite my youthful obnoxiousness: it was kind of silly to check afterwards if you could check before. Why check for an error afterwards if you can avoid it to begin with?

Passing null in production code is a very bad idea. It’s an example of what I call offensive programming – programming in way that encourages defensive programming. The better way to code is to deal with nulls as soon as they happen and translate them to null objects or exceptions. Unfortunately, this isn’t common knowledge.

Let’s look at an example.

Here’s a method that builds a set of parameters from an array of integers:

public String [] getParameters() {
    if (intParms.length  == 0) {
        return null;
    }
    String [] result = new String[intParms.length];
    for(int n = 0; n < intParms.length; n++) {
        result[n] = "parameter " + intParms[n];
    }
    return result;
}

Will everyone remember to check the return value for null? Probably not. Okay, let’s clean it up:

public String [] getParameters() {
    String [] result = new String[intParms.length];
    for(int n = 0; n < intParms.length; n++) {
        result[n] = "parameter " + intParms[n];
    }
    return result;
}

If we don’t have any parameters, we return an empty array. The empty array is an example of a null object.. an object which gracefully handles an exceptional case. Now that we're always returning a valid array, there's one less possible error.

When we program we always have the choice to program in a way which forces us to check for errors later or not. The alternative is finesse the problem, to design in a way that makes errors impossible. Making errors impossible is far better than handling them.

Let's revisit the original example:
private void addResolution(List<Flag> flags) {
    if (flags != null) {
        if (supportedAdapters.contains(Adapter.LOW) 
                || supportedAdapters.contains(Adapter.MEDIUM)) {
            flags.add(new ResolutionFlag(ADAPTED));
        } 
    }
} 

The method is private. If we go back to all of the callers we can change them so that flags can never be null. Then, we can clean up the code:

private void addResolution(List<Flag> flags) {
    if (supportedAdapters.contains(Adapter.LOW) 
            || supportedAdapters.contains(Adapter.MEDIUM)) {
        flags.add(new ResolutionFlag(ADAPTED));
    } 
} 

What we've done here is introduce a stupid-free zone in the code. If we tend it, it can grow.


James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding Posted: Jul 14, 2006 3:06 PM
Reply to this message Reply
What I think is good about your example is that it shows something that I often find to be true: that often the 'correct' way to write the code is simpler than the offensive version.

At my current employer I find that when I maintain a class, after cleaning up the formatting, I delete at least 10% of the members and method code because it's unecessary and very often causes problems.

Another example of this issue in Java is when a method returns a collection type and there several concerns:

Iteration requires explicit synchronization which when done can cause contention issues.

The caller needs a modifiable collection for their use. Trying to modify the returned collection could unintentionally modify the state of the source of the collection. If the source has wrapped the collection in a unmodifiable wrapper trying to modify it will result in a runtime exception.

The collection may be 'live' meaning changes to the source could modify the state of the returned collection. This can be true even if the returned collection is unmodifiable to the caller.

A quick solution to all of these is to take the returned collection and all all the elements to a new collection. The question is whether you do this in a defensive way or if the source should do this upfront. If one way is not chosen, you can end up doing it on both ends.

Michael Feathers

Posts: 448
Nickname: mfeathers
Registered: Jul, 2003

Re: Offensive Coding Posted: Jul 14, 2006 3:44 PM
Reply to this message Reply
> What I think is good about your example is that it shows
> something that I often find to be true: that often the
> 'correct' way to write the code is simpler than the
> offensive version.

Yeah, it seems that way to me too, but so much of it seems to be insight. When we have it, we can see the simpler thing. Without it, it all becomes brute force.

> At my current employer I find that when I maintain a
> class, after cleaning up the formatting, I delete at least
> 10% of the members and method code because it's unecessary
> and very often causes problems.
>
> Another example of this issue in Java is when a method
> returns a collection type and there several concerns:
>
> Iteration requires explicit synchronization which when
> done can cause contention issues.
>
> The caller needs a modifiable collection for their use.
> Trying to modify the returned collection could
> d unintentionally modify the state of the source of the
> collection. If the source has wrapped the collection in a
> unmodifiable wrapper trying to modify it will result in a
> runtime exception.
>
> The collection may be 'live' meaning changes to the source
> could modify the state of the returned collection. This
> can be true even if the returned collection is
> unmodifiable to the caller.
>
> A quick solution to all of these is to take the returned
> collection and all all the elements to a new collection.
> The question is whether you do this in a defensive way or
> r if the source should do this upfront. If one way is not
> chosen, you can end up doing it on both ends.


I just implicitly recoil from the idea of returning live collections of my innards. It can work if everyone understands that it is happening, but the potential for confusion is great.

I have this theory that the thing that differentiates good code bases from bad code bases is the fact that the good code bases have rules. Often they're not explicit, but they are easy to discern. I think it all goes back to the principle of least astonishment.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding Posted: Jul 14, 2006 4:21 PM
Reply to this message Reply
> I just implicitly recoil from the idea of returning live
> collections of my innards. It can work if everyone
> understands that it is happening, but the potential for
> confusion is great.

Yeah, I usually return a copy of the collection or if there's a really good reason to return a live collection, make it unmodifiable. One of the nice things about generics is that you can actually make the immutablity of your collection part of the API definition. This still doesn't solve the iterator synchronization issue though. Really, I think that's a result of a poor API design. The iterator should not be returned, rather a handler for each Object should be passed to the collection.

> I have this theory that the thing that differentiates good
> code bases from bad code bases is the fact that the good
> code bases have rules. Often they're not explicit, but
> they are easy to discern. I think it all goes back to the
> principle of least astonishment.

The problem I find with this is getting the rules together. My experience is that it's hard to get people to agree on the rules. I've also had the experience of coming into an organization where a lot of the rules were just wrong. For example, the coding standard said 'always use import package.*;' and that you could never import specific classes.

So I agree that rules help, It's getting the rules in place that seems to be the trouble.

piglet

Posts: 63
Nickname: piglet
Registered: Dec, 2005

Re: Offensive Coding Posted: Jul 14, 2006 4:24 PM
Reply to this message Reply
You gave an example of a private method but let's assume it is a public API method called all over the place. Is it really better that each client check the parameter before calling the method, instead of the method generically checking the input parameter? Certainly not! This method is eactly the right place to make the null check because it knows best what to do. It can throw an exception to tell the caller that they have made a serious mistake, or it can simply tolerate the null if it doesn't matter. Which btw is very often the case. I don't agree that null pointers should never be passed around. Null is a legitimate object. I agree however that excessive null checks stink. I prefer a coding style that concentrates null checks in as few places as possible. By far the most common null check is for strings, and usually it doesn't matter whether a string is null or empty. I absolutely hate seeing
 if (string!=null && !str.equals( ""))
, I just call
isEmpty( str)
. I also hate seeing
 if (parameter1!=null) {
         if (parameter2=null) {
            // 250 lines of code, including more nested null checks
         } else // error message "parameter2 invalid"
       } else // error message "parameter1 must not be null"


I'm seing this a lot and it's driving me crazy.


"Making errors impossible is far better than handling them." Well said, but please tell us how to "make errors impossible"!

piglet

Posts: 63
Nickname: piglet
Registered: Dec, 2005

Re: Offensive Coding Posted: Jul 14, 2006 4:40 PM
Reply to this message Reply
That should have been
 if (string!=null && !string.equals( ""))
.

Btw i would like to have a compiler-enforced not-null qualifier, as in Nice, for the cases where null is deemed illegal. As long as this doesn't exist, relying on clients to always pass valid parameters is a dangerous strategy. As Rusty Harold put it:
"Classes are responsible for enforcing their class invariants, not client programmers. Nobody who uses the public methods of a class should be assumed to understand what is and is not legal."

http://www.cafeconleche.org/XOM/designprinciples.xhtml

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding Posted: Jul 14, 2006 4:49 PM
Reply to this message Reply
> You gave an example of a private method but let's assume
> it is a public API method called all over the place. Is it
> really better that each client check the parameter before
> calling the method, instead of the method generically
> checking the input parameter?

I agree with you to a point but I think what Michael is saying is that neither would check for null.

I agree that nulls can be meaningful but in the case of collections, this is rarely the case.

> I absolutely
> hate seeing
> if (string!=null && !str.equals( "")), I
> just call isEmpty( str).
> I also hate seeing
> if (parameter1!=null) {
> if (parameter2=null) {

One of the ideas I've had for a OO language (that may already exist) would be a syntax for when 'this == null' allowing a behavior to be defined for when a method is called against a null but of course this wouldn't be polymorphic...

> // 250 lines of code, including more nested
> more nested null checks
> } else // error message "parameter2 invalid"
> } else // error message "parameter1 must not be
> not be null"
>
> I'm seing this a lot and it's driving me crazy.

My theory is that code like this is often a symptom of the 'single return' philisophy that is often espoused by university professors. Instead of checking the parameters up front and aborting if they are bad, the whole method is contorted to fufill this ideological requirement.

Michael Feathers

Posts: 448
Nickname: mfeathers
Registered: Jul, 2003

Re: Offensive Coding Posted: Jul 14, 2006 7:31 PM
Reply to this message Reply
> That should have been
 if (string!=null &&
> !string.equals( ""))
.
>
> Btw i would like to have a compiler-enforced not-null
> qualifier, as in Nice, for the cases where null is deemed
> illegal. As long as this doesn't exist, relying on clients
> to always pass valid parameters is a dangerous strategy.
> As Rusty Harold put it:
> "Classes are responsible for enforcing their class
> ass invariants, not client programmers. Nobody who uses
> the public methods of a class should be assumed to
> understand what is and is not legal."
>
> http://www.cafeconleche.org/XOM/designprinciples.xhtml

I disagree with that advice because I don't see all design as API design. I think it's unfortunate that much of the design advice that we see is geared towards API design and not application design.

In this specific case, I think that for most programming we should be able to assume that people aren't going to pass null parameters. To the degree that we can't, the application is messed up. It's just a matter of sanity.

Call it a global precondition.

Michael Feathers

Posts: 448
Nickname: mfeathers
Registered: Jul, 2003

Re: Offensive Coding Posted: Jul 14, 2006 7:41 PM
Reply to this message Reply
> You gave an example of a private method but let's assume
> it is a public API method called all over the place. Is it
> really better that each client check the parameter before
> calling the method, instead of the method generically
> checking the input parameter? Certainly not! This method
> is eactly the right place to make the null check because
> it knows best what to do. It can throw an exception to
> tell the caller that they have made a serious mistake, or
> it can simply tolerate the null if it doesn't matter.
> Which btw is very often the case. I don't agree that null
> pointers should never be passed around. Null is a
> legitimate object.

It's a sick little object, in my opinion. It makes programs go boom. NULL is even worse in C and C++.

> I agree however that excessive null
> checks stink. I prefer a coding style that concentrates
> null checks in as few places as possible.

I think that, again, it comes down to whether you're writing an API or not. If clients of your code are under your control, you can establish a safe subset coding convention: pass only non-null objects.

> "Making errors impossible is far better than handling
> them." Well said, but please tell us how to "make errors
> impossible"!

I don't think there is any algorithm for this. The thing I did above removes a possible error. The same thing can be said for using collection iterators rather than arrays, or using closures rather than collection iterators. I think that the key thing is to think of possible errors and wonder why they are there. Sometimes this involves going all the way back to requirements.

I was working on some code recently where I had to do some error checking because of an invalid state and I realized that it could be traced back to a particular requirement. If that requirement was altered a bit the error would be impossible. It didn't work out in that case, but when you do have control over the feature set, in something like new product development, you can often finesse away possible errors, so it's worth looking at.

Carfield Yim

Posts: 32
Nickname: carfield
Registered: Sep, 2002

Re: Offensive Coding Posted: Jul 15, 2006 12:19 AM
Reply to this message Reply
I probably always not return null except there is no other option.

However, if the parameter is kind of optional and you can have reasonable default, may be it is good to check null? As you never know how to caller do...

Keith Ray

Posts: 658
Nickname: keithray
Registered: May, 2003

Re: Offensive Coding Posted: Jul 15, 2006 10:38 AM
Reply to this message Reply
In Cocoa / Objective-C programming, I still prefer an empty string over a null pointer, or an empty container-object over a null object, but the language does have a key difference: calling object methods on null-pointers does not normally crash! (But sometimes method parameters being null can cause a crash.)

Foo* aFoo = nil;
Bar* aResult = [aFoo someMethod: anArgument ];

calling someMethod (as above) on a nil object DOES NOTHING (it does not crash or throw an exception) and returns nil.

The Cocoa UI frameworks take advantage of this. For example, the objects that implements a Button or a Menu item would have two members variables: a pointer to an object, and a "selector" that identifies what method to call on the object. The method that handles a click (or whatever) would be written something like this:

[ targetObject performSelector: actionSelector withArgument: self ];

instead of like this:

if ( targetObject != nil ) {
[ targetObject performSelector: actionSelector withArgument: self ];
}

No need to check for a null targetObject. There would be a need to check for a null-selector (in the performSelector:withArgument: method), since the selector isn't actually an object.

People have objected that null acting like the "null object pattern" hides bugs, but too many times have I gotten web-apps sending me messages that a "null pointer exception has occurred" so I assert that null-pointer exceptions are not helping people find bugs as well as test-driven development, or thorough code-reviews and testing, would do. I expect that if null had the "null object" behavior, the web-app would have returned an partial or empty result rather than a crashing message, and that would be fine for a most users.

If the "null = null object pattern" behavior is an expected and documented part of the language, it can work quite well. Cocoa and NextStep programmers are, and have been, very productive using a language with this behavior.

Werner Schulz

Posts: 18
Nickname: werner
Registered: May, 2005

Re: Offensive Coding Posted: Jul 15, 2006 6:28 PM
Reply to this message Reply
Null objects and similar cases are definitely a problem in code as it leads to too many checks. Certainly, in your own code you should strive to check arguments at the public method level and keep it sane inside.

But equally often I find it detrimental to work with basic objects like String, int, etc. since in most applications (unless it is basic API design) we should be dealing with domain-specific objects, which replace primitive objects like String, int, etc. Each value objects should have a defined domain of values, which normally would exclude null in almost all cases (or wrap it into a special value object a la NullObject pattern).

As soon as these values are agreed, you can forget all null values since null values are now clearly programming errors and should never (famous last words :) make it into production. At least that's would I try to strive for in code in my projects.

Derek Parnell

Posts: 22
Nickname: derekp
Registered: Feb, 2005

Re: Offensive Coding Posted: Jul 16, 2006 9:54 PM
Reply to this message Reply
I generally agree with the idea of trapping mistakes as early as possible. In your example code I see this as a logic error rather than a data error. In other words, it is illogical to pass a null object to this function; the function has the right to expect that only reasonable items will be passed to it. Of course, that situation would be different if the data being passed came directly from user-entered values, but in this case it comes from some other function that ought to be validating data prior to passing it along.

However, even though your example code is a private member, some form of defensive coding could be warranted in order to catch your own mistakes. Those languages that implement some form of Design by Contract (e.g. Eiffel and D) can help here.

For example, recoding this example in the D language...

private void addResolution(List!(Flag) flags) {
in {
assert(flags !is null, "addResolution got null flags");
}
body {
if (supportedAdapters.contains(Adapter.LOW)
||
supportedAdapters.contains(Adapter.MEDIUM)) {
flags.add(new ResolutionFlag(ADAPTED));
}
}
}

the in block is only executed in non-release editions of the application which allows you to perform the defensive code during in-house testing and to leave it out when the release edition of the application is built (using the -release switch).

Achilleas Margaritis

Posts: 674
Nickname: achilleas
Registered: Feb, 2005

Re: Offensive Coding Posted: Jul 17, 2006 4:25 AM
Reply to this message Reply
> <p>What’s wrong with it?</p>
>
> <p>No, it’s not the control flow. Sure, we could replace
> the conditional with a guard clause and get rid of some
> indentation, but the problem is more subtle than that.
> Most people are blind to it because it’s in their face
> e every day.</p>
>
> <p>The problem is this line:</p>
>
> <pre>if (flags != null)</pre>
>
> <p>Spurious null checks are a symptom of bad code.</p>

No, the problem with nulls is not their existence, but that there are not separate types. In functional programming languages, there is not a 'null' value, but there is 'nil', and the typechecker/pattern matching can catch that and create separate versions of code. For example, a maybe type can be defined like this:


type maybe T = T | nil


And it can be used like this:


fun doSomething T = bla bla handle case
| nil = bla bla handle null


So the property of 'maybe something exists' is encoded in the type system, and thus there is no need to do null checks or anything: if a pattern fails to match at compile time, the compiler will display an error.

The null problem clearly illustrates that C/C++/Java/C#/<put your favorite here> languages are seriously broken in this regard.

It also demonstrates the fact that values are types and not only groups of values. For example, the factorial function has two types:


fun factorial n = n * factorial (n - 1)
| 0 = 1


The above makes it clear that when it comes to factorial, number 0 is a different type than the rest of the numbers.

Many bugs in programs in the above mentioned languages are a result of values not being types. The knowledge of what values are allowed is carried within the head of the programmer, never given to the compiler...and thus compilers silently ignore many obvious errors.

Michael Stover

Posts: 28
Nickname: mstover
Registered: Jul, 2005

Re: Offensive Coding Posted: Jul 17, 2006 9:06 AM
Reply to this message Reply
If you had a rule "no nulls allowed", wouldn't you just be, in many cases, replacing logic like:

if(x != null)

with:

if(x.length() > 0)

or

if(x.length > 0)

or

if(x.size() > 0)

?

Certainly in some cases you are just iterating through the elements of the collection or array and if it's empty your code still works. But not always. Many times an empty collection/array means something special, and checking for it is no different than checking for null. Many times a zero-length string means the same as a null string, and requires special handling.

Another issue I see is that nulls go "all the way down". If you create an object with instance members, those members default to null. If you construct them, their members default to null. Either you are initializing all objects with all their instance members created as empty objects with default values (and zero-length strings), or you have to deal with null at some point. What about lazy instantiation/initialization?

Flat View: This topic has 67 replies on 5 pages [ 1  2  3  4  5 | » ]
Topic: Offensive Coding Previous Topic   Next Topic Topic: Hidden Variables


Sponsored Links



Google
  Web Artima.com   

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