The Artima Developer Community
Sponsored Link

Weblogs Forum
Offensive Coding

67 replies on 5 pages. Most recent reply: Sep 12, 2006 5: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 | » ]
Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Offensive Coding Posted: Jul 17, 2006 11:10 AM
Reply to this message Reply
Advertisement
>
> After many years of thinking about this, I've decided that
> I no longer want to be involved in software methodologies
> that cater to the lowest-common-denominator of developers.
> I'm on more of a 'improve your work or get out of the
> e way' kick lately.
>

A lot of the code I develop has custom applications written against it. A lot of people that do these customizations shouldn't be near a keyboard and if I could I would break their fingers with a baseball bat. Needless to say, they don't let me walk around with a bat at work anymore :-(

> > The real problem, as you point out, is failing
> silently.
> > And I don't think that you need to worry much about the
> > caller's intent. If your code is truly encapsulated it
> > should have an idea about what constitutes good input
> and
> > bad input and then act accordingly. And the user should
> be
> > informed in some manner that leads to a way to resolve
> the
> > issue.
>
> I agree with that but I don't think it needs to be
> micro-managed in every method.

I don't disagree with that but that is so context dependent that I can't help but have issues with a sweeping generalization like "Spurious null checks are a symptom of bad code." As much as I probably sound like a whiny prick, it's a testament to the respect I have for Michael Feathers that I even read the rest of the article. My big problem with that is that where a lot of people would stop reading at that statement and that's why I get so fired up about blogs and articles like this. If you read through the whole thing and think about it and apply it to your context there may be some cases where it applies. If you read it and all you remember is "Spurious null checks are a symptom of bad code." then you go into your code base and remove all those null checks and suddenly things stop working and you get into a big argument about it with your manager or the other developers and what's your recourse? I read it on Micheal Feathers' blog and he's a well known XP guy so what he says must be right? What if you come back with Michael's 'stupid free zone' idea? Have you just called your boss stupid to his/her face? Been there, done that, wouldn't recommend it :-)

>And all the talk about "well, my flight code might get
> passed a null and I don't trust the callers" really
> bothers me. If I were writing such code (I dont), there
> sure as heck better be extensive testing that they ARE
> passing in correct parameters.

How on earth can you have a fault tolerant system if you assume all the input you ever get is going to be good? It may bother you but just a few weeks ago there was an incident of a flight that just stopped operating midflight for a few seconds. Nobody got hurt and nothing bad happened but the plane just stopped responding to any of the pilot's input. One component in the flight control system did not like the input that it got from another. Components on airplanes are not all updated at the same time and while they do go through rigorous testing the reality of the situation is that you cannot possibly test all combinations of inputs and outputs for all the different systems that could be working together in all their possible combinations. Everybody seems to agree with this but the agile and XP crowd is BIG on hacking up the simplest thing that will work and testing with very little, if any, emphasis on logically examining the code and verifying that it will do what it should.

And here are some real doozies if you care to read about them.

http://www.baselinemag.com/print_article2/0,1217,a=120920,00.asp

Google for "famous software bugs" and you'll get some really neat results. This is one of my favorites because nobody died:

USS Yorktown (1998)

A crew member of the guided-missile cruiser USS Yorktown mistakenly entered a zero for a data value, which resulted in a division by zero. The error cascaded and eventually shut down the ship's propulsion system. The ship was dead in the water for several hours because a program didn't check for valid input. (reported in Scientific American, November 1998)

I wonder if the person that wrote that could have pleaded that his code should have been running in a stupid free zone? We may not be working on radiation treatment systems or missle guidance systems or systems that run missile cruisers but what makes you think that the people that are working on these systems are much different than you or me? I interviewed for several of those types of jobs when I graduated from college and even got an offer from one of them. My best friend worked for Sikorsky as a summer intern and worked on a radar system for one of their helicopters for the military. I've seen his code. It's better than most I've seen but he writes bugs just like the rest of us. Heck I've fixed a few of them.

You say "If I were...". What makes you think you would do anything differently than you do now? Experience has made me a big believer in the mantra "How you practice is how you play." If you think that you would be doing something differently than you are now but you aren't taking steps to do things in that manner, or aren't currently doing things in that manner, then I don't see how you can say that. More likely you would be doing things exactly the same whether you are working on CRUD applications or missile guidance systems or video games. Or you would be doing things some other way and you would be really, really miserable.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding Posted: Jul 17, 2006 11:32 AM
Reply to this message Reply
> USS Yorktown (1998)
>
> A crew member of the guided-missile cruiser USS Yorktown
> mistakenly entered a zero for a data value, which resulted
> in a division by zero. The error cascaded and eventually
> shut down the ship's propulsion system. The ship was dead
> in the water for several hours because a program didn't
> check for valid input. (reported in Scientific American,
> November 1998)

This is a very good example for discussion.

> I wonder if the person that wrote that could have pleaded
> that his code should have been running in a stupid free
> zone?

Well, I think that's a bit of a stretch because the idea is that we are talking about internal code, not human input. You can never count on humans, no matter how much some people argue that their behavior is absolutely deterministic.

But here's the thing. The method that tried to divide by zero, what should it do? Does it assume the caller meant 1? That's kind of like if you work at a quarry and are waiting for authorization to set off explosive, get a phone call that you don't understand and based on that, fire the charge.

To me the point here is that if you write code such that it will accept null pointers, people will pass them to you. Michael is not saying don't check for null and then throw an error. He saying don't check for null and treat is as equivalent to empty.

The Yorktown example, to me, is actually a good example of how exceptions shine. In a system with proper top-level exception handling, this error would have come back to the person doing data entry even if the GUI writers didn't do their job and check for valid input. They might not understand the error but at least it wouldn't have crashed the system.

Moreover the Yorktown example is an endictment of the team that developed it. They clearly didn't do their due diligence in testing it. They probably had the developers test it themselves.

Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Offensive Coding Posted: Jul 17, 2006 11:37 AM
Reply to this message Reply
> OK. Sorry for the knee-jerk reaction. I agree that
> structured programming has provent itself. Even in the
> cases that Knuth argued warranted gotos, equivalent
> structured approaches have been developed notably:
> exceptions.

That's ok. I would have said the exact same thing. :-) I re-read what I wrote after your response and had an "Oh, duh" moment.

Pamela Selby

Posts: 1
Nickname: pammy
Registered: Jul, 2006

Re: Offensive Coding Posted: Jul 17, 2006 12:27 PM
Reply to this message Reply
Hopefully this message will not qualify as "Offensive Coding".

I am a recruiter at Robert Half Technology and have 3 opportunities in the Atlanta area: 1 Architect and 2 Sr. Developers. The company is a well recognized company and is willing to be competitive with salary.

I can provide a $500 referral fee for each selected candidates, so please have your friends give me a call as soon as possible.

Pam Selby
Robert Half Technology
404-233-1382
pamela.selby@rht.com

piglet

Posts: 63
Nickname: piglet
Registered: Dec, 2005

Conetxt is everything Posted: Jul 17, 2006 1:22 PM
Reply to this message Reply
I can't help but have issues with a sweeping generalization like "Spurious null checks are a symptom of bad code."...

If your code is truly encapsulated it should have an idea about what constitutes good input and bad input and then act accordingly...

I'm interested in my program acting correctly on any input. There should rarely, if ever, be any 'maybe' about what is passed in. You should be able to fail to a reasonable state when you get bad input. You should be able to figure out if the input you are getting is good or not and you should be able to figure out what to do in either case. The amount of processing you do in this regard is totally dependent on the system you are writing.


Again, that is nicely put, Brandy. The message is really that context is everything and generalizations are not helpful.

I agree that failing silently is bad. I would further say that, if you are concerned with failing silently then your time would be better spent getting rid of all those empty try/catch blocks because of checked exceptions than they would be looking for spurious null checks.

Except that empty try/catch blocks are not caused by checked exceptions. They are caused by cluesless programmers.

You get a NullPointerException instead of a InvalidArgumentException ("null parameter"). Is there really a difference?

Again, it depends on the context. First, the null pointer may not be bad data, just a special case that requires special treatment (like, doing nothing). If it is bad data and results immediately in a NullPointerException, this may be perfectly appropriate. However, the error is often propagated and results in some unexpected exception further down the road, which makes it more difficult to fix. The InvalidArgumentException makes sense in those cases to the extent that it helps making debugging easier. It is hardly more "graceful" than NullPointerException.

There is also the case when the null pointer does not provoke an exception although it should, as in your example when orders were created with corrupted data. That would be a validation failure - the order system should have validated the data (including but not restricted to null checks) before placing orders. There are again different cases - is it a user interface validating manual input, an API validating parameters passed by clients? The more you think about it, the more obvious it is how much everything depends on the context. Are you hearing me, Michael?

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Conetxt is everything Posted: Jul 17, 2006 1:46 PM
Reply to this message Reply
> You get a NullPointerException instead of a
> InvalidArgumentException ("null parameter"). Is there
> really a difference?

>
> Again, it depends on the context. First, the null pointer
> may not be bad data, just a special case that requires
> special treatment (like, doing nothing). If it is bad data
> and results immediately in a NullPointerException, this
> may be perfectly appropriate. However, the error is often
> propagated and results in some unexpected exception
> further down the road, which makes it more difficult to
> fix. The InvalidArgumentException makes sense in those
> cases to the extent that it helps making debugging easier.
> It is hardly more "graceful" than NullPointerException.

You know, now that you mention it, being able to specify that null parameter should cause an exception in the method parameter syntax would be nice. Having the compile verify that no null could ever be passed is a bit of overkill (this is what I was imagining you meant before) but just a little sugar that would also be visible as part of the signature would be nice.

> There is also the case when the null pointer does not
> provoke an exception although it should, as in your
> example when orders were created with corrupted data. That
> would be a validation failure - the order system should
> have validated the data (including but not restricted to
> null checks) before placing orders. There are again
> different cases - is it a user interface validating manual
> input, an API validating parameters passed by clients? The
> more you think about it, the more obvious it is how much
> everything depends on the context. Are you hearing me,
> Michael?

Again, I think people are reading into this a little. Think about it this way. You come along to maintain a class that uses a certain private variable throughout it's methods. This variable is set by a call to a setter method. Prior to that, it is null. Every method that uses this value checks to see that it's non-null before using it. The point being made here (I think) is that you can restructure the code so that the variable is never null. Then the rest of the code becomes a lot cleaner.

Gregor Zeitlinger

Posts: 108
Nickname: gregor
Registered: Aug, 2005

Re: Offensive Coding Posted: Jul 17, 2006 1:49 PM
Reply to this message Reply
> I would argue that the NullPointerException is the "was I
> stupid flag" and that if flight-control systems are
> passing bad parameters around then I don't want to fly.
Then you probably do not want to fly (ignoring the fact that airplanes probably do not use Java).

I would not want to fly in an airplane that cannot tolerate faults.

In other words, you cannot build a reliable system if you simply try to eliminate all of your bugs. Your application must work in an acceptable way even though it has bugs.

In Java, you have to catch all exceptions at a point where you can return to a defined state.

For an airlpane, that would be no sensible strategy.
You will probably want to have multiple programs that each decide how to steer the airplane - let the majority decide.

Michael Feathers

Posts: 448
Nickname: mfeathers
Registered: Jul, 2003

Re: Offensive Coding Posted: Jul 17, 2006 1:55 PM
Reply to this message Reply
> I don't disagree with that but that is so context
> dependent that I can't help but have issues with a
> sweeping generalization like "Spurious null checks are a
> symptom of bad code." As much as I probably sound like a
> whiny prick, it's a testament to the respect I have for
> Michael Feathers that I even read the rest of the article.
> My big problem with that is that where a lot of people
> would stop reading at that statement and that's why I get
> so fired up about blogs and articles like this.

Yeah, I don't know what to do about it either. The thing is, I thought that I was avoiding misunderstanding by saying this in my blog:

"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, people will generalize. I don't know what to do about that. It seems that it makes it very hard to talk about some things.

Michael Feathers

Posts: 448
Nickname: mfeathers
Registered: Jul, 2003

Re: Conetxt is everything Posted: Jul 17, 2006 2:03 PM
Reply to this message Reply
> Again, I think people are reading into this a little.
> Think about it this way. You come along to maintain a
> a class that uses a certain private variable throughout
> it's methods. This variable is set by a call to a setter
> method. Prior to that, it is null. Every method that
> uses this value checks to see that it's non-null before
> using it. The point being made here (I think) is that you
> can restructure the code so that the variable is never
> null. Then the rest of the code becomes a lot cleaner.

Thanks, James. That's exactly the point I was making.

One thing that does get me about this topic, though, is the difference between what we say and what we do in the industry. I have never seen a code base where all parameters on all methods are checked for null. And I have seen a very large number of code bases. That's not to say that there aren't a couple, but I haven't run into them yet.

Erik Engbrecht

Posts: 210
Nickname: eengbrec
Registered: Apr, 2006

Re: Offensive Coding Posted: Jul 17, 2006 2:04 PM
Reply to this message Reply
>The Yorktown example, to me, is actually a good example of
>how exceptions shine. In a system with proper top-level
>exception handling, this error would have come back to the
>person doing data entry even if the GUI writers didn't do
>their job and check for valid input. They might not
>understand the error but at least it wouldn't have crashed
>the system.

Yes, GUI writers should validate input.

No, model writers should not assume that the data received from the view is accurate.

In fact, I'd go so far as to say that the model should NEVER enter an inconsistent state without throwing an exception that makes it clear that the client just did something so bad it invalidated the model.

Think about this. How may classes do you have with mutable hashcodes (meaning the hashcode changes when you change certain properties)? Do you store these objects in sets or use them as keys in part of your model? When an operation is invoked that will change the hashcode, do you update every HashSet that contains the object, and throw an exception is one of those operations fails?

If a client tries to invoke a method with parameters that will invalidate the model (meaning anything in the object graph), and exception should be thrown.

Ok, I'll stop ranting. You get the idea.

Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Conetxt is everything Posted: Jul 17, 2006 2:43 PM
Reply to this message Reply
> > Again, I think people are reading into this a little.
> > Think about it this way. You come along to maintain a
> > a class that uses a certain private variable throughout
> > it's methods. This variable is set by a call to a
> setter
> > method. Prior to that, it is null. Every method that
> > uses this value checks to see that it's non-null before
> > using it. The point being made here (I think) is that
> you
> > can restructure the code so that the variable is never
> > null. Then the rest of the code becomes a lot cleaner.
>
> Thanks, James. That's exactly the point I was making.
>
> One thing that does get me about this topic, though, is
> the difference between what we say and what we do in the
> industry. I have never seen a code base where all
> parameters on all methods are checked for null. And I
> have seen a very large number of code bases. That's not
> to say that there aren't a couple, but I haven't run into
> them yet.

I won't say that I check every parameter that goes into every method, but neither would I advise to not check something. I've already had a couple people come by today (I don't know how many people I've recommended this site to...) and we've talked about this article. They either didn't get do the part you quote above or it slipped their mind from the time they read it to the time they asked my opinion. So I had to find out where they found that advice. And here I am, ranting.

On the plus side, it gave me some interesting reading and writing to do.

Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Offensive Coding Posted: Jul 17, 2006 3:12 PM
Reply to this message Reply
> > I don't disagree with that but that is so context
> > dependent that I can't help but have issues with a
> > sweeping generalization like "Spurious null checks are
> a
> > symptom of bad code." As much as I probably sound like
> a
> > whiny prick, it's a testament to the respect I have for
> > Michael Feathers that I even read the rest of the
> article.
> > My big problem with that is that where a lot of people
> > would stop reading at that statement and that's why I
> get
> > so fired up about blogs and articles like this.
>
> Yeah, I don't know what to do about it either. The thing
> is, I thought that I was avoiding misunderstanding by
> saying this in my blog:
>
> "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, people will generalize. I don't know what to do
> about that. It seems that it makes it very hard to talk
> about some things.


What happened to the rest of the quote? "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."

How is that not a generalization on your part? People pass null == bad code is what I see there. As I said in a previous post, I've had several people come by and talk to me about this today and what they got from it is "Don't check for null". I've also had to fix plenty of code that would qualify as "stupid free" because it didn't check for null and then subsequently crashed, telling me that an instance of an object was not instantiated. So it had to get fixed. And, honestly, I don't have the time to refactor an entire code base to take care of an issue where somebody didn't make sure their input was valid, so the input gets validated in that instance where it is crashing. If I had to refactor every codebase I came across to be what I would consider "right", well, I don't want to work that long. :-(

There are also several instances in code I work on where null is a valid occurence and very different from an object, such as in your array example, that has no entries. In these cases we had to reduce overhead (mostly free memory that containted nothing but lots of empty structures) and pick up some performance. I keep hearing how arguments like these are red herrings and straw men, yet when I implement these changes I get from 15 - 60% performance improvements. Projects like this are why I agree with your assertion in theory but not in practice, as I also said.

And yeah, these things can be hard to talk about. Especially when you call people stupid. Or, maybe, more technically, tell people they write code that resides in a stupid zone. I have, and will continue to write, a lot of code that you would probably consider stupid, simply because I don't know where or how it is going to get used. Most of the code I've written I have almost 0 control over how it gets used so I have to be VERY defensive in my coding practices. So maybe I took your jab a little personally.

Erik Engbrecht

Posts: 210
Nickname: eengbrec
Registered: Apr, 2006

Re: Offensive Coding Posted: Jul 17, 2006 3:45 PM
Reply to this message Reply
Just rereading your article after ranting.

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

I think this is too broad of a statement. Null means an absence of value, as opposed to an empty string or collection. If null has a valid semantic meaning, it should be used.

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

You should go back a bold this part.

Russ Freeman

Posts: 7
Nickname: freemanr
Registered: Jul, 2006

Re: Offensive Coding Posted: Jul 17, 2006 4:39 PM
Reply to this message Reply
Oh, I see so much of this type of coding at the moment and when I ask why, I always get the same response "..but what if someone passes null?".

It's just a plain case of not agreeing your contract up front in any form. Either the function accepts nulls or it doesn't. End of story. Usually, the only time a function should accept nulls is to specify optionality, i.e. you can trace that null check back to some requirement.

With any system, it's useful to distinguish between the "pure core" where the contracts are understood and adhered to and the outer interface layers that communicate with external domains. It is only this interface layer that needs to deal with hostile input, checking for nulls, if this layer is an API or checking for blanks, in the case of a UI. But once the input is checked, it can be passed to the core without further checks.

I guess another way to think about this is a nudist beach. You wouldn't expect to get arrested for climbing over a rock and scaring some old lady. If the old lady is already on the beach, she's already agreed to lose the clothes, right? ;)

So the way you treat nulls, and indeed the way to report violations, differs depending if you are talking about core code or the interface code. In the former case, it is usually an assertion and the latter, a runtime exception for an API or user message for a UI.

Keith Ray

Posts: 658
Nickname: keithray
Registered: May, 2003

Re: Offensive Coding Posted: Jul 17, 2006 4:49 PM
Reply to this message Reply
"How on earth can you have a fault tolerant system if you assume all the input you ever get is going to be good?"

No one suggested no checking of input. What I believe Michael and some others are advocating is separation of concerns. Separate validation of input from processing of input.

In the simplest non-OO example:

x = input from UI

if isvalid( x ) then // check for NULLs or other bad data in "isvalid"
process ( x ) // don't need to check for NULLS or other bad data in "process"


Probably 90% of problems in code, including NULL issues, comes from improper separation of concerns and "primitive obsession".

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-2019 Artima, Inc. All Rights Reserved. - Privacy Policy - Terms of Use