c# - 将先决条件分组到可以接受的方法中以保持 DRY 吗？

Question

在为具有相似参数的不同函数编写前提条件时，我想将断言或异常分组到一个静态方法中，而不是显式地写出来。例如，而不是

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
}

我更愿意写

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);
  ...
}

这感觉更自然，有助于减少我需要编写的代码(甚至可能减少我的断言中的错误数量!)但它似乎违背了前提条件应该有助于记录方法的确切意图的想法.

这是一种常见的反模式还是有什么重要的理由不这样做？

此外，如果我使用异常，答案会有所不同吗？

Answer 1

可靠的答案

这是绝对可以接受的:.NET 源代码包含条件。

例如，StringBuilder source code有一个名为 VerifyClassInvariant() 的方法，它调用了 18 次。该方法只是检查正确性。

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;
    }
}

对话框

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

是的。没关系。

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

包装assert 或Exception 语句的方法的名称应该传达包装器的意图。此外，如果读者想了解具体细节，那么她可以查看该方法的实现(除非它是闭源的。)

Is this considered good or bad practice, and why?

将一组相关的或经常重用的 assert 包装在另一个方法中是一种很好的做法，因为它既可以提高可读性，又可以方便维护。

Is this a common anti-pattern?

实际上恰恰相反。您可能会看到类似这样的内容，它实际上很有帮助并值得推荐，因为它可以很好地沟通。

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

    // other conditions omitted
}

将 ThenThrow 添加到方法的末尾是个好主意，这样调用者就知道您正在抛出。否则，您可能希望返回一个 bool 并让调用者决定在条件失败时要做什么。

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

    // other conditions omitted
}

然后调用代码可以抛出:

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

或者，这里是 another example from the .NET source如果某些条件失败则抛出异常(ThrowHelper 只是抛出异常。)

// 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?

我能想到的是，如果方法名称没有解释您正在检查的内容，并且没有一种简单的方法来查看源代码，那么您应该避免包装条件。