Является ли группировка предварительных условий приемлемым для пребывания 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; 
    } 
} 

Dialog

Это хорошо группы утверждений или исключений в статический метод, а не записывать их в явном виде?

Да. Все нормально.

... похоже, противоречит идее о том, что предпосылки должны помочь документировать точное намерение метода.

Имя метода, который оборачивает assert или Exception заявления должны сообщаться намерения обертки. Кроме того, если читатель хочет знать специфику, то она может рассмотреть реализацию метода (если только это не является закрытым источником).

Считается ли это хорошей или плохой практикой и почему?

Это хорошая практика, чтобы обернуть набор связанных или обычно повторно используемых asserts другим способом, поскольку он может улучшить читаемость и облегчить обслуживание.

Это общий анти-шаблон?

Противоположный, на самом деле. Вы можете увидеть что-то подобное, и это действительно полезно и рекомендуется, потому что оно хорошо сообщает.

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, что бросает исключение, если некоторые условия не

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

( ThrowHelper просто выбрасывает исключения.)

Есть ли существенные причины не делать этого?

Что я могу придумать, если имя метода не объясняет, что вы проверяете, и нет простого способа просмотра источника, тогда вам, вероятно, следует избегать условий обертывания.