Is grouping preconditions into a method acceptable to stay DRY?

Question

When writing preconditions for different functions with similar parameters, I want to group assertions or exceptions into a static method, rather than writing them out explicitly. For example, instead of

GetFooForUser(User user) 
{
  assert(null != user);
  assert(user.ID > 0); // db ids always start at 1
  assert(something else that defines a valid user);
  ...

  // do some foo work
}

GetBarForUser(User user) 
{
  assert(null != user);
  assert(user.ID > 0); // db ids always start at 1
  assert(something else that defines a valid user);
  ...

  // do some bar work
}

I would prefer to write

GetFooForUser(User user) 
{
  CheckUserIsValid(user);

  // do some foo work
}

GetBarForUser(User user) 
{
  CheckUserIsValid(user);

  // do some bar work
}

static CheckUserIsValid(User user)
{
  assert(null != user);
  assert(user.ID > 0); // db ids always start at 1
  assert(something else that defines a valid user);
  ...
}

This feels more natural and helps cut down the code I need to write (and might even reduce the number of errors in my assertions!) but it seems to go against the idea that preconditions should help document the exact intent of a method.

Is this a common anti-pattern or are there any significant reasons not to do this?

Also, would the answer be different if I had used exceptions?

Answer 1

Solid Answer

It's absolutely acceptable: the .NET Source Code wraps conditions.

For instance, the StringBuilder source code has a method called VerifyClassInvariant() that it calls 18 times. The method just checks correctness.

private void VerifyClassInvariant() 
{    
    BCLDebug.Correctness((uint)(m_ChunkOffset + m_ChunkChars.Length) >= m_ChunkOffset, "Integer Overflow");
    StringBuilder currentBlock = this;
    int maxCapacity = this.m_MaxCapacity;
    for (; ; )
    {
        // All blocks have copy of the maxCapacity.
        Contract.Assert(currentBlock.m_MaxCapacity == maxCapacity, "Bad maxCapacity");
        Contract.Assert(currentBlock.m_ChunkChars != null, "Empty Buffer");

        Contract.Assert(currentBlock.m_ChunkLength <= currentBlock.m_ChunkChars.Length, "Out of range length");
        Contract.Assert(currentBlock.m_ChunkLength >= 0, "Negative length");
        Contract.Assert(currentBlock.m_ChunkOffset >= 0, "Negative offset");

        StringBuilder prevBlock = currentBlock.m_ChunkPrevious;
        if (prevBlock == null)
        {
             Contract.Assert(currentBlock.m_ChunkOffset == 0, "First chunk's offset is not 0");
             break;
        }
        // There are no gaps in the blocks. 
        Contract.Assert(currentBlock.m_ChunkOffset == prevBlock.m_ChunkOffset + prevBlock.m_ChunkLength, "There is a gap between chunks!");
        currentBlock = prevBlock;
    }
}

Dialog

Is it okay to group assertions or exceptions into a static method, rather than writing them out explicitly?

Yes. It's okay.

... it seems to go against the idea that preconditions should help document the exact intent of a method.

The name of the method that wraps the assert or Exception statements ought to communicate the wrapper's intent. Also, if the reader wants to know the specifics, then she can view the method's implementation (unless it's closed-source.)

Is this considered good or bad practice, and why?

It's good practice to wrap a set of related or commonly reused asserts in another method, because it can both improve readability and facilitate maintenance.

Is this a common anti-pattern?

The opposite, actually. You might see something like this and it's actually helpful and recommended because it communicates well.

private void IfNotValidInputForPaymentFormThenThrow(string input)
{
    if(input.Length < 10 || input.Length)
    {
        throw new ArgumentException("Wrong length");
    }

    // other conditions omitted
}

It's a good idea to add ThenThrow to the end of the method so that the caller knows you're throwing. Otherwise, you might want to return a bool and let the caller decide what to do if the condition fails.

private bool IsValidInputForPaymentForm(string input)
{
    if(input.Length < 10 || input.Length)
    {
        return true;
    }
    else
    {
        return false;
    }

    // other conditions omitted
}

Then the calling code can throw:

if(!IsValidInputForPaymentForm(someStringInput)
{
    throw new ArgumentException();
}

Or, here is another example from the .NET source that throws an exception if some conditions fail (the ThrowHelper just throws exceptions.)

// Allow nulls for reference types and Nullable<U>, 
// but not for value types.
internal static void IfNullAndNullsAreIllegalThenThrow<T>(
    object value, ExceptionArgument argName) 
{
    // Note that default(T) is not equal to null 
    // for value types except when T is Nullable<U>. 
    if (value == null && !(default(T) == null))
        ThrowHelper.ThrowArgumentNullException(argName);
}

are there any significant reasons not to do this?

What I can think of is, if the method name does not explain what you're checking, and there isn't an easy way to view the source, then you should probably avoid wrapping conditions.