views:

680

answers:

15

I was having a discussion with one of my colleagues about how defensive your code should be. I am all pro defensive programming but you have to know where to stop. We are working on a project that will be maintained by others, but this doesn't mean we have to check for ALL the crazy things a developer could do. Of course, you could do that but this will add a very big overhead to your code.

How do you know where to draw the line?

+10  A: 

I tend to change the amount of defense I put in my code based on the language. Today I'm primarily working in C++ so my thoughts are drifting in that direction.

When working in C++ there cannot be enough defensive programming. I treat my code as if I'm guarding nuclear secrets and every other programmer is out to get them. Asserts, throws, compiler time error template hacks, argument validation, eliminating pointers, in depth code reviews and general paranoia are all fair game. C++ is an evil wonderful language that I both love and severely mistrust.

JaredPar
OK, I get your point... I'm working in C++ too and I totally agree: it is evil :P. But doesn't that make your code less readable?
@Ionut, No, I don't feel so. I don't use any hacky syntax or crazy #ifdef's to assert. I usually wrap the checks in functions or function style macros. Like ThrowIfNull, ThrowIfFalse, TemplateUtil.CompileTypeAssertIsChild<Type1,Type2>(). All are very readable and add much needed context information into how I expect various values to be used.
JaredPar
A: 

Defensive programming is only one way of hounouring a contract in a design-by-contract manner of coding.

The other two are

  • total programming and
  • nominal programming.

Of course you shouldnt defend yourself against every crazy thing a developer could do, but then you should state in wich context it will do what is expected to using preconditions.

//precondition : par is so and so and so 
function doSth(par) 
{
debug.assert(par is so and so and so )
//dostuf with par 
return result
}
Peter
+2  A: 

If you're working on public APIs of a component then its worth doing a good amount of parameter validation. This led me to have a habit of doing validation everywhere. Thats a mistake. All that validation code never gets tested and potentially makes the system more complicated than it needs to be.

Now I prefer to validate by unit testing. Validation definitely happens for data coming from external sources, but not for calls from non-external developers.

Frank Schwieterman
+3  A: 

I don't know that there's really any way to answer this. It's just something that you learn from experience. You just need to ask yourself how common a potential problem is likely to be and make a judgement call. Also consider that you don't necessarily have to always code defensively. Sometimes it's acceptable just to note any potential problems in your code's documentation.

Ultimately though, I think this is just something that a person has to follow their intuition on. There's no right or wrong way to do it.

Jason Baker
+1 can't replace experience and intuition with any classes at school! :-)
marc_s
I agree. Intuition is probably the best way. But intuition comes from experience. What if you don't have enough experience? You just follow the standards of the project?
@Ionut Anghelcovici - there's only one way to get experience: the school of hard knocks. The best way to know what kinds of mistakes should be avoided is to make them yourself. You need to be willing to screw up horrendously. Remember, on most software projects, mistakes can be fixed.
Jason Baker
+12  A: 

Anything a user enters directly or indirectly, you should always sanity-check. Beyond that, a few asserts here and there won't hurt, but you can't really do much about crazy programmers editing and breaking your code, anyway!-)

Alex Martelli
+1: Don't trust any user input ! EVER!
marc_s
The software that I'm working on has very drastic security requirements, so the sanity-check for user input is a must. My problem is that something that might seem crazy for me can look normal for another dev. So, from my point of view, an assert shouldn't be needed, but another dev might expect one
@Ionut Anghelcovici - Much as I hate to both advocate Hungarian notation *and* make the obligatory Joel on Software link, you can fix that using *proper* Hungarian notation: http://www.joelonsoftware.com/articles/Wrong.html
Jason Baker
A: 

I think you have to bring in the question of whether you're creating tests as well. You should be defensive in your coding, but as pointed out by JaredPar -- I also believe it depends on the language you're using. If it's unmanaged code, then you should be extremely defensive. If it's managed, I believe you have a little bit of wiggleroom.

If you have tests, and some other developer tries to decimate your code, the tests will fail. But then again, it depends on test coverage on your code (if there is any).

MunkiPhD
+1  A: 

I always Debug.Assert my assumptions.

VVS
+7  A: 

I'm not a fan of the term "defensive programming". To me it suggests code like this:

void MakePayment( Account * a, const Payment * p ) {
    if ( a == 0 || p == 0 ) {
       return;
    }
    // payment logic here
}

This is wrong, wrong, wrong, but I must have seen it hundreds of times. The function should never have been called with null pointers in the first place, and it is utterly wrong to quietly accept them.

The correct approach here is debatable, but a minimal solution is to fail noisily, either by using an assert or by throwing an exception.

Edit: I disagree with some other answers and comments here - I do not think that all functions should check their parameters (for many functions this is simply impossible). Instead, I believe that all functions should document the values that are acceptable and state that other values will result in undefined behaviour. This is the approach taken by the most succesful and widely used libraries ever written - the C and C++ standard libraries.

And now let the downvotes begin...

anon
I'm cursed with a mind that always adds "Well, there might be some rare, specific situation...." Though I can't think of one for your "bad example."
RolandTumble
@Neil Your problem isn't with defencive programming, it is with BAD defencive programming. The correct way for MakePayment to react is to throw a NullReferenceException in this case, and good Defencive Programming would say that you should wrap MakePayment in a Try-Catch. Defencive Programming is what protects you from edge cases like the one you've highlighted.
Like devinb said, I would change the return to instead throw an exception, since this is clearly an exceptional circumstance and in this case, I don't think that its bad since its up to the function to verify the preconditions, imho
Dan
+1  A: 

My personal ideology: the defensiveness of a program should be proportional to the maximum naivety/ignorance of the potential user base.

gnovice
man.. you send him on a journey to find the boundaries of human stupidity?? he wanted to know where the line is, not that it is at infinity. My advice: Put in some reasonable assertions, take the blue pill, pray to any gods you find, cross your fingers and commit your code...
AndreasT
@AndreasT: Haha! Good one. However, in all seriousness, software intended for use by a bunch of developers probably wouldn't need the same sort/number of checks or speed bumps as, say, software intended for clueless end-users.
gnovice
A: 

I try to write code that is more than defensive, but down right hostile. If something goes wrong and I can fix it, I will. if not, throw or pass on the exception and make it someone elses problem. Anything that interacts with a physical device - file system, database connection, network connection should be considered unereliable and prone to failure. anticipating these failures and trapping them is critical

Once you have this mindset, the key is to be consistent in your approach. do you expect to hand back status codes to comminicate problems in the call chain or do you like exceptions. mixed models will kill you or at least drive you to drink. heavily. if you are using someone elses api, then isolate these things into mechanisms that trap/report in terms you use. use these wrapping interfaces.

MikeJ
A: 

If the discussion here is how to code defensively against future (possibly malevolent or incompetent) maintainers, there is a limit to what you can do. Enforcing contracts through test coverage and liberal use of asserting your assumptions is probably the best you can do, and it should be done in a way that ideally doesn't clutter the code and make the job harder for the future non-evil maintainers of the code. Asserts are easy to read and understand and make it clear what the assumptions of a given piece of code is, so they're usually a great idea.

Coding defensively against user actions is another issue entirely, and the approach that I use is to think that the user is out to get me. Every input is examined as carefully as I can manage, and I make every effort to have my code fail safe - try not to persist any state that isn't rigorously vetted, correct where you can, exit gracefully if you cannot, etc. If you just think about all the bozo things that could be perpetrated on your code by outside agents, it gets you in the right mindset.

Coding defensively against other code, such as your platform or other modules, is exactly the same as users: they're out to get you. The OS is always going to swap out your thread at an inopportune time, networks are always going to go away at the wrong time, and in general, evil abounds around every corner. You don't need to code against every potential problem out there - the cost in maintenance might not be worth the increase in safety - but it sure doesn't hurt to think about it. And it usually doesn't hurt to explicitly comment in the code if there's a scenario you thought of but regard as unimportant for some reason.

jjb
+1  A: 

Being defensive against developers consuming your API code is not that different from being defensive against regular users.

  • Check the parameters to make sure they are within appropriate bounds and of expected types
  • Verify that the number of API calls which could be made are within your Terms of Service. Generally called throttling it usually only applies to web services and password checking functions.

Beyond that there's not much else to do except make sure your app recovers well in the event of a problem and that you always give ample information to the developer so that they understand what's going on.

Chris Lively
But there is a big difference between a regular user and a dev. A regular user will ignore documentation and try inputs that are wrong and expect the application to ignore/correct.A dev must be more skilled and able to understand the documentation. If he gets a crash/strange behavior he should be able to find the problem, not expect me to make sure his code is perfect :)
Your kidding right? I haven't run into very many dev's that actually read the documentation first. And the ones that do consult it before plunging ahead only skim to the points they are interested in. Usually they just grab some sample source and plug it right into their environment. When things start failing is when they refer to the docs.
Chris Lively
BTW, as proof that dev's don't read the documentation; a huge number of questions on this site are solved by simply reading the published docs on the component in question.
Chris Lively
A: 

Systems should have well designed boundaries where defensive checking happens. There should be a decision about where user input is validated (at what boundary) and where other potential defensive issues require checking (for example, third party integration points, publicly available APIs, rules engine interaction, or different units coded by different teams of programmers). More defensive checking than that violates DRY in many cases, and just adds maintenance cost for very little benifit.

That being said, there are certain points where you cannot be too paranoid. Potential for buffer overflows, data corruption and similar issues should be very rigorously defended against.

Yishai
A: 

I recently had scenario, in which user input data was propagated through remote facade interface, then local facade interface, then some other class, to finally get to the method where it was actually used. I was asking my self a question: When should be the value validated? I added validation code only to the final class, where the value was actually used. Adding other validation code snippets in classes laying on the propagation path would be too defensive programming for me. One exception could be the remote facade, but I skipped it too.

A: 

Good question, I've flip flopped between doing sanity checks and not doing them. Its a 50/50

situation, I'd probably take a middle ground where I would only "Bullet Proof" any routines that are:

(a) Called from more than one place in the project

(b) has logic that is LIKELY to change

(c) You can not use default values

(d) the routine can not be 'failed' gracefully

Darknight

Darknight