Coding style - Input validation

Question

Which is the best way of validating an input passed to the function i.e. do you validate all input before proceeding some thing like

class A;
void fun(A* p)
{
  if(! p)
  {
    return;
  }

 B* pB = p->getB();
  if(! pB)
  {
    return;
  }

.......

}

Or do you write it like this:

void fun(A* p)
{
  if(p)
  {
    B* pB = p->getB();
    if(pB)
    {
      .....
    }
  }
}

I am asking this because, if I use the first style then I'll have multiple return statements in my code which many people say are bad (don't know why) and if I use the second style then there will be too many levels of nesting in my code.

Answer 1

I prefer to do my input validation upfront, but will often perform the tests all at once

i.e.

if(!p || !p->getB()){
    return;
}

If the input is required for the function to operate, and the language supports it, I throw Argument Exceptions (see .NET ArgumentNullException, ArgumentException) so that the code doesn't do clean returns in the case of invalid states. This lets the caller know that the arguments were not sufficient to complete the operation.

Answer 2

The first way is easier to read and less complex (by depth) than the second one. In the second one, the complexity and depth increase as the number of parameters goes up. But in the first example, it's just linear.

Answer 3

Ideally, you'd pass by reference and then you wouldn't have to worry about this as much. That's not always an option, though. Otherwise it's a matter of preference.

I prefer the first, personally, as I don't really have much of a problem with multiple returns. I think that functions should basically fit on one or two screens anyway, so as long as you have syntax highlighting it shouldn't be hard to see all of a function's exit points.

Answer 4

I use the first method you posted, which allows me, in most cases, to provide either: meaningfull return values or messages to the user:

btnNext_OnClick(...)
{
if(!ValidateData())
 return;

// do things here
}


boolean ValidateData()
{
if(!condition)
{
 MessageBox.Show("Message", "Title", MessageBoxButtons.OK, MessageBoxIcons.Information);
 return false;
}

return true;
}

That way, when I add a field that needs form level validation, I can add another if statement check to the validation method.

Answer 5

I prefer the first, to the point where I have a set of macros to handle the common cases (and typically assert at the same time for debugging purposes). The downside of this approach is that your function return types have to be fairly uniform for your macro usage to be uniform; the upside is that the usage/effect is very clear. So, for example, my code might read:

class A;
void fun(A* p)
{
  ASSERT_NONZERO_RETURN_ON_FAIL( p );

  B* pB = p->getB();
  ASSERT_NONZERO_RETURN_ON_FAIL( pB );

  .......
}

This leads to much more readable code which also alerts you to errors. Also, as an added bonus, you can easily disable to checking in a release build if you find you value the marginal speed increase over the value of the runtime checks.

Added note: In my experience, the reason some people say multiple return points from functions are bad is because they are doing resource de-allocation explicitly before function exit, and thus you don't want to duplicate the de-allocation code. If you're using RIIA correctly and uniformly, though, this should not be an issue. As this is what I try to always do also, multiple return points is preferable to nesting for me.

Answer 6

Multiple returns: people and coding standards go both ways. in C++, imo there is no reason not to prefer multiple returns (and exceptions), if you are using RAII etc. Many C coding standards enforce single-entry-single-exit to make sure all cleanup code gets executed, because there is no RAII and scope-based destruction/resource cleanup.

http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement

Answer 7

I tend to use these type of checks and returns at the beginning of the method. So that I sort of emulate Eiffel like design by contract preconditions and assertions, as described in Bertrand Meyer's book Object-Oriented Software Construction, second edition, Prentice Hall.

For your example, instead of returning void, I would return an enum identifying the violation something like:

enum Violation { inviolate = 0, p_unspecified_unavailable, p_B_unspecified_unavailable,..... };

Violation fun(A* p)
{
//__Preconditions:___________________________________
  if (! p)         return p_unspecified_unavailable:
  if (! p->getB()) return pB_unspecified_unavailable;
//__Body:____________________________________________
   ....
//__Postconditions: _________________________________
   ..(if any)..
   return inviolate;
}

My point being that, I consider the precondition (and any post condition) validation to be a wrapping around implementation of the body of method/function and tend to distinguish and separate the flow of control conditional logic from the expressions of the body.