Weblogs Forum - Offensive Coding

Articles |
News |
Weblogs |
Books |
Forums

Artima Forums | Articles | Weblogs | Java Answers | News

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

Back to Topic List

Reply to this Topic

Search Forum

Threaded View


Previous Topic		Next Topic

Flat View: This topic has 67 replies on 5 pages [ « | 1 2 3 4 5 | » ]

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 6:47 AM

> Another issue I see is that nulls go "all the way down".
> If you create an object with instance members, those
> e 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.

This is a case of offensive coding. It is also an encapsulation issue. As much as possible, I always initialize variables upon construction and mark everything I can as final. If I can't initialize it, I make sure that the behavior is reasonable when the variable is not set to anything. But really, this should not be a common issue in good OO code. The problem I see is that people use Objects as glorified structures and write procedural code around them.

Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Offensive Coding

Posted: Jul 17, 2006 7:36 AM

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

This is one of those areas where I agree in theory, but not in practice. Who said sanity has anything to do with the situation? Besides, the person calling the function may not be knowingly passing a null paramter when they write the code. They may be passing in an object they got from another function call, system call or third party library that they thought would not be null but in some odd case on the second Tuesday of May when the moon is full, null gets returned and then the flight control system of the airplane you are riding in blows up because it does not know what to do with null input and instead of arriving safely in Amsterdam to enjoy a weekend of debauchery you are floating in the middle of the Atlantic after a 'water landing' and then you get your legs bitten off by a hungry mako shark.

Personally, I'll continue to check for null when I need to. Without context your simple code sample doesn't convince me that the null check is spurious. In fact, your 'was I stupid flag!' sounds like a very important thing in the case of, say, a flight recorder. Calling to check the flag may not be useful but having that information certainly is in many circumstances. Maybe, given that your method is private in the example, I'll buy that you can make sure everything that calls it isn't passing null. But then you have to ask how much you trust the people that are going to come after you.

While this may be a fun theoretical exercise, I think in the real world this is just bad advice. If that makes me stupid, fine. I can deal with that. Personally I don't trust any input I get. I've just gotten too much bad input in my life to trust it without examining it first.

piglet

Posts: 63
Nickname: piglet
Registered: Dec, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 7:52 AM

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

That's right. In the getParameters() example, it would be wrong to return null because the semantics of the method clearly demands that a valid collection be always returned. Confusing "empty collection" with null is a serious mistake. If that is Michael's point, I agree. However if he's saying that null objects should in general never be passed around, I think he's going too far. It depends on the context whether an uninitialized object makes legitimate sense or not, and I don't know any simple rules by which to decide. In principle, it should be specified for each method whether null as input parameter or return value is meaningful - if not explicitly then at least mentally. However, an explicit Javadoc note is preferable. Even better would be a compiler-enforced qualifier.

My theory is that code like this is often a symptom of the 'single return' philosophy 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.
Yes. I never understood the appeal of this "single return" ideology.

piglet

Posts: 63
Nickname: piglet
Registered: Dec, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 7:57 AM

Personally, I'll continue to check for null when I need to. Without context your simple code sample doesn't convince me that the null check is spurious. In fact, your 'was I stupid flag!' sounds like a very important thing in the case of, say, a flight recorder. Calling to check the flag may not be useful but having that information certainly is in many circumstances. Maybe, given that your method is private in the example, I'll buy that you can make sure everything that calls it isn't passing null. But then you have to ask how much you trust the people that are going to come after you.

While this may be a fun theoretical exercise, I think in the real world this is just bad advice. If that makes me stupid, fine. I can deal with that. Personally I don't trust any input I get. I've just gotten too much bad input in my life to trust it without examining it first.

Exactly. Thanks for summing up so nicely.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 8:09 AM

> I agree that nulls can be meaningful but in the case of
> collections, this is rarely the case.
>
> That's right. In the getParameters() example, it would be
> wrong to return null because the semantics of the method
> clearly demands that a valid collection be always
> returned. Confusing "empty collection" with null is a
> serious mistake. If that is Michael's point, I agree.
> However if he's saying that null objects should in general
> never be passed around, I think he's going too far.

I don't think that's his point but I could be projecting my thoughts onto his. I think his point is that there are a lot of cases where we check for nulls merely because we are afraid someone will do something 'offensive' like pass in a null. His argument is that instead of trying to resolve this after the error is made that we should resolve the error at the source of the bad data.

> It depends on the context whether an uninitialized object
> makes legitimate sense or not, and I don't know any simple
> rules by which to decide.

I don't think there are many simple rules in programming. Whenever someone comes up with one, it's almost always wrong in some context.

> My theory is that code like this is often a symptom of
> the 'single return' philosophy 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.
> Yes. I never understood the appeal of this "single return"
> ideology.

It obstensibly makes the code less complicated because 'you always know where the return is' or something like that. My contention is that it does the opposite and makes code more complicated and harder to understand and verfiy. Mainly because overlapping execution paths destroy information. Suppose the question was "is the parameter bad because I need to use it at this point?" With a single-return paradigm in place, the asnwer is, "can't know until runtime, let's check." In the multiple-return paradigm, the answer is "parameter is not bad because method execution does not reach this point if parameter is had."

I've maintained a lot of code developed with the single-return paradigm and it's extremely difficult to follow and tends to be much more buggy than multiple-return code.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 8:23 AM

> This is one of those areas where I agree in theory, but
> not in practice. Who said sanity has anything to do with
> the situation? Besides, the person calling the function
> may not be knowingly passing a null paramter when they
> write the code. They may be passing in an object they got
> from another function call, system call or third party
> library that they thought would not be null but in some
> odd case on the second Tuesday of May when the moon is
> full, null gets returned and then the flight control
> system of the airplane you are riding in blows up because
> it does not know what to do with null input and instead of
> arriving safely in Amsterdam to enjoy a weekend of
> debauchery you are floating in the middle of the Atlantic
> after a 'water landing' and then you get your legs bitten
> off by a hungry mako shark.

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.

The problem with the, "I'll rescue you" programming is that there is no way to know what the caller meant when they passed in the bad parameter. In the flight control system example, maybe the null parameter meant 'empty' maybe it's a bug and treating it as empty will cause the program to fail silently and the plane to crash, whereas blowing up will let the pilot know that the flight-system has failed. More importantly, silent failures are a lot harder to detect during testing and I'm pretty sure flight control systems are tested pretty well.

My experience is that a lot of bugs that make it into production are the result of not blowing up when things go wrong. For example, I had a bad day where I had to manually merge some code with a contractor's (don't ask) and I screwed up. I didn't notice the issue in limited testing (because I stupdily decided it was already tested.) What happened next was extremely ugly. I was passing in bad data, instead of failing up front and rejecting the input, it placed orders in the system with corrupted data. It took quite a while for the data to percolate through the system and make the problem apparent, so meanwhile tens of thousands of bad orders were being placed.

If the code had just blown up on bad input 1. I wouldn't have put it into production and 2. even if I had, the problems would have been manifested immediately with failed orders resulting in a short downtime instead of a huge debacle.

Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Offensive Coding

Posted: Jul 17, 2006 8:32 AM

> Yes. I never understood the appeal of this "single return"
> ideology.

Does nobody study the history of Computer Science anymore?

Some guy named Dijkstra thought a single point of exit was a good idea when he and others started talking about structured programming. By narrowing the focus of what a function did and applying rigorous structure to a program (single point of entry, single point of exit, no gotos, etc.) one could approach computer programs with the same type of mindset that one approached mathematical proofs. At the time the folks that came up with the idea of structured programming it was a HUGE leap forward in approach to how programs should be developed. Back in the day goto was how you escaped just about any executable block and resulted in steaming piles of code so high it would make your head spin.

While there were disagreements about the details (Single point of exit being one of the biggies. Donald Knuth disagrees with single point of exit.), the overall idea has, with time, been proven correct I believe.

Much as I despise linking to wikipedia, a good overview of structured programming can be found there http://en.wikipedia.org/wiki/Structured_programming

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 8:35 AM

> While this may be a fun theoretical exercise, I think in
> the real world this is just bad advice. If that makes me
> stupid, fine. I can deal with that. Personally I don't
> trust any input I get. I've just gotten too much bad input
> in my life to trust it without examining it first.

I also do this most of the time but I see Michael's point here. This is similar to a problem I have on my current team: We spend a lot more time creating monitors for processes and manually resolving issues than actually fixing the root causes of issues. While this may seem more efficient at first, it really kills us in the long run because the issues keep occuring again and again and the overhead of creating the monitor is often greater than writing the code properly in the first place.

Think for a second if you started just allowing null pointers to be thrown. What would happen? Other people passing bad data would cause the code to blow up. Then the people who passed in the bad data would have to fix it. After a while, they'll stop passing in bad data. If you are working with them, you can help them to understand how to avoid creating the bad data in the first place.

It's kind of like an article I read about database normalization. The author makes a good case that there is no inherent reason that normalized data cannot be retrieved efficiently and then points out that even if this isn't the case right now, keeping the database denormalized only lets the DBMS vendors off the hook. If everyone demanded good performance with denormalized data, the vendors would surely provide it.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 8:40 AM

> While there were disagreements about the details (Single
> point of exit being one of the biggies. Donald Knuth
> disagrees with single point of exit.), the overall idea
> has, with time, been proven correct I believe.

Single-exit has been proven wrong as far as I am concerned. It was proven wrong to me by those who believed it with their code that made my life hell.

There's also nothing more annoying than the 'that programming control structure is a goto' argument. All program execution control structures are formalized gotos.

Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Offensive Coding

Posted: Jul 17, 2006 9:01 AM

>
> The problem with the, "I'll rescue you" programming is
> that there is no way to know what the caller meant when
> they passed in the bad parameter. In the flight control
> system example, maybe the null parameter meant 'empty'
> maybe it's a bug and treating it as empty will cause the
> program to fail silently and the plane to crash, whereas
> blowing up will let the pilot know that the flight-system
> has failed. More importantly, silent failures are a lot
> harder to detect during testing and I'm pretty sure flight
> control systems are tested pretty well.
>
> My experience is that a lot of bugs that make it into
> production are the result of not blowing up when things go
> wrong. For example, I had a bad day where I had to
> manually merge some code with a contractor's (don't ask)
> and I screwed up. I didn't notice the issue in limited
> testing (because I stupdily decided it was already
> tested.) What happened next was extremely ugly. I was
> passing in bad data, instead of failing up front and
> rejecting the input, it placed orders in the system with
> corrupted data. It took quite a while for the data to
> percolate through the system and make the problem
> apparent, so meanwhile tens of thousands of bad orders
> were being placed.
>
> If the code had just blown up on bad input 1. I wouldn't
> have put it into production and 2. even if I had, the
> problems would have been manifested immediately with
> failed orders resulting in a short downtime instead of a
> huge debacle.

I'm not interested in rescuing anybody else. 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. 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.

While your story is bad (This makes a much better case than the original post to me, but lacks sweeping generalizations about spurious null checks), from the customer's standpoint it probably did the "right" thing. Bad orders, while painful to deal with, can be fixed. If I was a customer and I was inputting an order into an order entry system and I got something that said "Null Pointer Exception" I would immediately go on to use your competitor's product and probably never return to yours. If I ordered "Harry Potter and the Chamber of Secrets" and I recieved "Potting Plants by Harry Chambers" I would call up the seller, tell them they sent me the wrong book, get it corrected and then go on my way. In your case, if a lot of users happened to have failed orders in that short downtime, your users would likely be shopping for or with a competitor's product.

The real problem with the example in the original post is not that it checks for null but that it doesn't do anything if it detects the bad case. Instead of going on to do nothing (I guess that makes it a spurious null check) it should let you know in a graceful manner what went wrong. This is where I guess I totally disagree with the Michael's post. He goes on to explain all the ways to just get rid of the null check and says that you have to trust the input because you are in a stupid free zone. Problem is, there are not stupid free zones. Even if we're not stupid we sometimes make mistakes. You don't sound stupid to me but you have a brilliant example of something that can be called stupid.

Assuming you will never get bad input is, in my experience, a huge risk. That's like saying you should drive without a seatbelt. Hey, I've had no accidents. I'm a totally competent driver. I never speed. I always use my turn signal. I have never, in my life, broken one traffic rule. Guess what, that seat belt isn't there because you don't know how to drive. It's there because out there somewhere is some person who doesn't. Or, more to the point, he doesn't drive like you. I think programming is like that in a lot of ways. People that don't program the way you do are wrong or stupid or pick your favorite adjective. I know quite a few people that would read Michael Feather's article and say

"WOOHOO, I don't need to worry about bad input. I'll just put in little comments in my code saying

// Stupid-Free zone. Do not pass in any bad input

and then get back to reading Artima."

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.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 9:25 AM

> I'm not interested in rescuing anybody else. 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. 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.

If the method doesn't throw an exception on bad input, how will that help?

> While your story is bad (This makes a much better case
> than the original post to me, but lacks sweeping
> generalizations about spurious null checks), from the
> customer's standpoint it probably did the "right" thing.
> Bad orders, while painful to deal with, can be fixed.

Actually, they could only be fixed manually, at great expense.

> If I
> was a customer and I was inputting an order into an order
> entry system and I got something that said "Null Pointer
> Exception" I would immediately go on to use your
> competitor's product and probably never return to yours.

That's not even in the realm of possibility. When there is a problem, orders would queue up and the system would panic. It's impossible to have the system be 100% up at all times so handling of failures is built into the system.

> If I ordered "Harry Potter and the Chamber of Secrets" and
> I recieved "Potting Plants by Harry Chambers" I would call
> up the seller, tell them they sent me the wrong book, get
> it corrected and then go on my way. In your case, if a lot
> of users happened to have failed orders in that short
> downtime, your users would likely be shopping for or with
> a competitor's product.

Again, not at all. This is a B2B system that handles high-volumes of electronic ordering. The user in this case is a machine. And it's not a case of ordering the wrong thing (which would actually be disasterous because we would end up paying for millions of dollars worth of lost merchandise. The products are shipping to a third party and it's problematic to get thousands of businesses to pay to ship back heavy items back to a company they have never heard of.

What happend in reality was that the system processed the orders but the shipping carrier code was garbage so the warehouses basically dead-lettered thousands of orders. Because the warehouse is automated, resolving each of these manually is a lot more expensive.

If the the code had not placed the order and just blew up, the issue would have been corrected within an hour and few outside my team would even be aware of the issue. The customer probably wouldn't have know or cared about it.

> The real problem with the example in the original post is
> not that it checks for null but that it doesn't do
> anything if it detects the bad case. Instead of going on
> to do nothing (I guess that makes it a spurious null
> check) it should let you know in a graceful manner what
> went wrong.

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

> [...snip...]
>
> Assuming you will never get bad input is, in my
> experience, a huge risk. That's like saying you should
> drive without a seatbelt. Hey, I've had no accidents. I'm
> a totally competent driver. I never speed. I always use my
> turn signal. I have never, in my life, broken one traffic
> rule. Guess what, that seat belt isn't there because you
> don't know how to drive. It's there because out there
> somewhere is some person who doesn't. Or, more to the
> point, he doesn't drive like you.

In the case of driving, you have no control over what other drivers do. A lot of development projects are like this but they don't have to be.

I think programming is
> like that in a lot of ways. People that don't program the
> way you do are wrong or stupid or pick your favorite
> adjective. I know quite a few people that would read
> Michael Feather's article and say
>
> "WOOHOO, I don't need to worry about bad input. I'll just
> put in little comments in my code saying
>
> // Stupid-Free zone. Do not pass in any bad input
>
> and then get back to reading Artima."

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 way' kick lately.

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

Morgan Conrad

Posts: 307
Nickname: miata71
Registered: Mar, 2006

Re: Offensive Coding

Posted: Jul 17, 2006 9:30 AM

Even though Sun says not to do this, I've got an autographed copy of Bertrand Meyer's book and the "correct and true DBC" :-) approach is to add the line

assert foo != null

Turn on assertion checking during development and do TDD. Turn off assertion checks in the production code.

Since Sun says not to do this, I'm guessing nobody else does it, nor do they believe in Meyer. Why?

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.

Merriodoc Brandybuck

Posts: 225
Nickname: brandybuck
Registered: Mar, 2003

Re: Offensive Coding

Posted: Jul 17, 2006 10:14 AM

> > While there were disagreements about the details
> (Single
> > point of exit being one of the biggies. Donald Knuth
> > disagrees with single point of exit.), the overall idea
> > has, with time, been proven correct I believe.
>
> Single-exit has been proven wrong as far as I am
> concerned. It was proven wrong to me by those who
> believed it with their code that made my life hell.
>
> There's also nothing more annoying than the 'that
> programming control structure is a goto' argument. All
> program execution control structures are formalized gotos.

I meant structured programming in general, not single exits. Sorry if that wasn't clear. Single exits can cause lots of pain.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 10:35 AM

> > > While there were disagreements about the details
> > (Single
> > > point of exit being one of the biggies. Donald Knuth
> > > disagrees with single point of exit.), the overall
> idea
> > > has, with time, been proven correct I believe.
> >
> > Single-exit has been proven wrong as far as I am
> > concerned. It was proven wrong to me by those who
> > believed it with their code that made my life hell.
> >
> > There's also nothing more annoying than the 'that
> > programming control structure is a goto' argument. All
> > program execution control structures are formalized
> gotos.
>
> I meant structured programming in general, not single
> exits. Sorry if that wasn't clear. Single exits can cause
> lots of pain.

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.

James Watson

Posts: 2024
Nickname: watson
Registered: Sep, 2005

Re: Offensive Coding

Posted: Jul 17, 2006 10:36 AM

> Even though Sun says not to do this, I've got an
> autographed copy of Bertrand Meyer's book and the "correct
> and true DBC" :-) approach is to add the line
>
> assert foo != null
>
> Turn on assertion checking during development and do TDD.
> Turn off assertion checks in the production code.
>
> Since Sun says not to do this, I'm guessing nobody else
> does it, nor do they believe in Meyer. Why?

Where does Sun say not to do this? They just added the assertion feature in 1.4 and it defaults to 'assertions off' at runtime. Why would they add such a feature if they didn't want people to use it?

Flat View: This topic has 67 replies on 5 pages [ « | 1 2 3 4 5 | » ]

Previous Topic

Next Topic