Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I've been wanting to find a solid answer to the question of whether or not to have runtime checks to validate input for the purposes of ensuring a client has stuck to their end of the agreement in design by contract. For example, consider a simple class constructor:

class Foo
{
public:
  Foo( BarHandle bar )
  {
    FooHandle handle = GetFooHandle( bar );
    if( handle == NULL ) {
      throw std::exception( "invalid FooHandle" );
    }
  }
};

I would argue in this case that a user should not attempt to construct a Foo without a valid BarHandle. It doesn't seem right to verify that bar is valid inside of Foo's constructor. If I simply document that Foo's constructor requires a valid BarHandle, isn't that enough? Is this a proper way to enforce my precondition in design by contract?

So far, everything I've read has mixed opinions on this. It seems like 50% of people would say to verify that bar is valid, the other 50% would say that I shouldn't do it, for example consider a case where the user verifies their BarHandle is correct, but a second (and unnecessary) check is also being done inside of Foo's constructor.

share|improve this question

migrated from stackoverflow.com Apr 9 '12 at 17:27

This question came from our site for professional and enthusiast programmers.

    
possible duplicate of How should I handle invalid user input? –  gnat Jul 29 '13 at 9:23

3 Answers 3

up vote 8 down vote accepted

I don't think there is a single answer to this. I think the main thing that's necessary is consistency -- either you enforce all preconditions on a function, or else you don't try to enforce any of them. Unfortunately, that's fairly rare -- what typically happens is that instead of thinking about the preconditions and enforcing them, programmers add bits of code to enforce preconditions whose violation happened to cause failures during testing, but frequently leave other possibilities open that may cause failure but didn't happen to arise in testing.

In many cases, it's quite reasonable to provide two layers: one for "internal" use that makes no attempt at enforcing any preconditions, and then a second for "outside" use that just enforces preconditions, then invokes the first.

I do, however, think it's better to have the preconditions enforced in the source node, not just documented. An exception or assert is much harder to ignore than documentation and a lot more likely to stay synchronized with the rest of the code.

share|improve this answer
    
In principle, I agree with your last paragraph. Although that now means that there are three things that need to be kept in sync; the documentation, the asserts themselves, and the test cases that prove the asserts are doing their job (if you believe in such things)! –  Oliver Charlesworth Apr 9 '12 at 17:33
    
@OliCharlesworth: Yes, it does create a third thing to keep in sync, but it establishes one (the enforcement in the source code) as the one that's generally trusted when there's disagreement. Otherwise, you generally don't know. –  Jerry Coffin Apr 9 '12 at 17:37
1  
@JerryCoffin I could check if foo is NULL, but being NULL isn't the only way foo could be invalid. For example, what about -1 cast to a FooHandle? I can't verify all possible ways the handle could be invalid. NULL is an obvious choice and something that is typically checked for, but not a conclusive check. What would you recommend here? –  void.pointer Apr 9 '12 at 18:40
    
@RobertDailey: Ultimately, it's almost impossible to assure against every possible abuse, especially when/if casting gets involved. With casting, the user can subvert essentially anything you can check. What I'm mostly stressing is the difference between 1) assuming parameters are good, and adding checks for the things that go wrong in testing, and 2) figuring out the preconditions as accurately as you can, and enforcing them as well as you can. –  Jerry Coffin Apr 9 '12 at 18:45
    
@JerryCoffin How does this differ from Defensive Programming, which is generally not considered a "good thing"? Most of the time, defensive programming techniques like this and many others I've seen are not very pragmatic. It's a design made to fight your co-workers bad coding habits or other things instead of focusing on practical functionality and implementation of your methods. I just see this easily getting out of hand as a habit that adds extra boiler-plate logic all over class functions. You'd think unit testing eliminates the need for these precondition checks. –  void.pointer Apr 9 '12 at 18:51

A quote I've heard on this is:

"Be conservative in what you do and liberal in what you accept."

Which boils down to following the contracts for arguments when you call functions, and checking all the inputs before acting when you write functions.

Ultimately it depends on the domain. If you're doing an OS API, you'd better check every input, don't trust all incoming data as valid before you start acting on it. If you're doing a library for others to use, go ahead, let the user screw themselves (OpenGL comes to mind first for some unknown reason).

EDIT: in the OO sense, there seems to be two approaches- one that says an object should never be malformed (all its invariants must be true) for the entire time that an object is accessable, and another approach that says you have a constructor which doesn't set all invariants, then you set a few more values and have a second initialization function that finishes init.

I kind of like the former better since it doesn't require magic knowledge or rely on current documentation to know what parts of initialization the constructor doesn't do.

share|improve this answer
    
For such initialization I prefare builder object that holds partial initialization data and later creates fully inicialized "usefull" object. –  user470365 Apr 10 '12 at 10:18

It's a very difficult question, because there are several different concepts:

  • Correctness
  • Documentation
  • Performance

However this is mostly an artifact of a type fault, in this case. Nullity is better enforced by type constraints, because the compiler actually checks those. Still, because not everything can be captured in a type system, especially in C++, the question itself is still worth it.


Personally, I think that correctness and documentations are paramount. Being fast and wrong is useless. Being fast and only wrong sometimes is a bit better, but does not bring much to the table either.

Performance though may be critical in some parts of the programs, and some checks can be quite extensive (ie: prove that a directed graph has all its nodes both accessible and co-accessible). Therefore I would vote for a dual approach.

Principle one: Fail Fast. This is a guiding principle in defensive programming in general, which advocates detecting errors at the earliest stage possible. I would add Fail Hard to the equation.

if (not bar) { abort(); }

Unfortunately, in a Production environment failing hard is not necessarily the best solution. In this case a specific exception can help getting out of there in a hurry, and let some high-level handler catch on and deal with the failed case appropriately (most likely logging and forging ahead with a new case).

This however does not address the issue of expensive tests. In hot spots, those tests may cost too much. In this case, it is reasonable to only enable the test in DEBUG builds.

This leaves us with a nice and simple solution:

  • SOFT_ASSERT(Cond_, Text_)
  • DEBUG_ASSERT(Cond_, Text_)

Where the two macros are defined thusly:

 #ifdef NDEBUG
 #  define SOFT_ASSERT(Cond_, Text_)                                                \
        while (not (Cond_)) { throw Exception(Text_, __FILE__, __LINE__); }
 #  define DEBUG_ASSERT(Cond_, Text_) while(false) {}
 #else // NDEBUG
 #  define SOFT_ASSERT(Cond_, Text_)                                                \
        while (not (Cond_)) {                                                       \
            std::cerr << __FILE__ << '#' << __LINE__ << ": " << Text_ << std::endl; \
            abort();                                                                \
        }
 #  define DEBUG_ASSERT(Cond_, Text_) SOFT_ASSERT(Cond_, Text_)
 #endif // NDEBUG
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.