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?

Frantz

I added a definitive answer. – Hammack

For me, a primary difference would be the scope of the precondition checking. In this case (because it's a complex business rule - "something else that defines a valid user" - being checked) I would have no objection to a 'wrapped/helper' check. However, I would probably check null separately. – Heerlen

@Heerlen Why would you check null separately? I have a number of grouped checks that require null checking as the first step anyway. – Frantz

@JensEhrich I mean null of the variable itself in the caller; because this increases an informal visual "can't be null" contract. If using annotations or Code Contracts this fits in more naturally. (One of my biggest gripes with C# is nullable types.) – Heerlen

@JensEhrich That is, the null check relates to the variable/argument (in the caller) and not the validity of the contents of said variable, should an object be present. – Heerlen

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.

Hammack

I have updated my question to be slightly less opinion-based. Your point about naming will probably be the deciding factor for me. I'll consider it acceptable when the intent of the entire group can be conveyed in the name. – Frantz