Should I redundantly test arguments (e.g collection emptiness)?

Question

Is there a problem with the redundant collection checking here?:

SomeMethod()
{
    shapes = GetShapes();
    //maybe Assert(shapes.Any())?
    if(shapes.Any())
    {
        ToggleVisibility(shapes);
    }
}

ToggleVisibility(IEnumerable<Shape> shapes)
{
    //maybe Assert(shapes.Any())?
    if(shapes.Any())
    {
        //do stuff 
    }
}

Answer 1

If you're adding those assertions for testing and debugging, sure, that makes sense.

In those situations, you want to be told when things don't go the way you expect them always to go.

In production, however, you probably don't want to tank the whole application by making calls on non-existent members of the shapes collection.

Answer 2

You can use Code Contract Library. In this case you can dynamically configure preconditions (validating incoming values), postconditions (validating results) and invariants (conditions that must be always true for a particular class) in your code.

Answer 3

I don't think there's a big problem here because calling Any() is not an expensive operation.

There is a minor problem in that the responsibility and behavior of ToggleVisibility is not declared. ToggleVisibility should let callers know how it will behave if shapes is empty or null. The best way to do this is through XML comments so that it shows up in Intellisense. This will let ToggleVisibility callers decide if they need to check if the collection is empty or null.

Answer 4

I think the key here is knowing the responsibility. If you know every single place that will ever call ToggleVisibility and intend to always check before hand then it is fine to not check in the ToggleVisibility method.

For my part I would check it inside ToggleVisibility because it makes the caller code cleaner and if you call the ToggleVisibility function from 50 different places then you have considerably less code.

Answer 5

I would suggest that the answer is... as usual... "it depends". While calling Any on an IEnumerable is not expensive, is it really necessary? That depends on what you are planning on doing with your collection in the method.

Will your method throw an exception, or something else undesirable, because of an empty collection? Are you iterating over your collection with a foreach? If so, then having an empty collection wouldn't necessarily do any harm, though it may be against your business rules. Trying to iterate over a null collection is obviously different.

You use GetShapes() as an example framework for an answer. To expand on my idea, is it really illegal to ToggleVisibility() on an empty collection? It obviously won't do much, but if the user highlighted an empty set of shapes, and then clicked on the toggle visibility function, would it do anything bad?

Answer 6

If ToggleVisibility(IEnumerable<Shape>) is a private method (thus SomeMethod() must be in the same library), then I would definitely include the check only one time in the Release build. Whether the check is in one method or the other depends on what makes sense for what is happening. If the collection is expected to never be empty in a correct execution, then perhaps no check is needed. If ToggleVisibility(IEnumerable<Shape>) is being called from ten different places, and any of them may have an empty collection, then I would definitely relieve the caller of the burden of doing the check every time, and just stick it inside the method itself.

If ToggleVisibility(IEnumerable<Shape>) is part of a public API, then it should definitely do whatever parameter validation is necessary, since users of APIs are likely to do anything, and all parameters must be checked at all times. If the documentation for the method states that empty collections will be ignored, then SomeMethod() does not need to worry about it, obviously. Otherwise, SomeMethod() needs to do whatever it takes to verify that the collection that it is passing is valid, even if that means that redundant checks are made.