Il raggruppamento di precondizioni in un metodo accettabile per rimanere DRY?

Question

Quando si scrivono precondizioni per funzioni diverse con parametri simili, voglio raggruppare asserzioni o eccezioni in un metodo statico, piuttosto che scriverle esplicitamente. Ad esempio, invece di

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 
} 

io preferirei scrivere

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

questo si sente più naturale e aiuta a ridurre il codice ho bisogno di scrivere (e potrebbe anche ridurre il numero di errori nelle mie asserzioni !) ma sembra andare contro l'idea che le condizioni preliminari dovrebbero aiutare a documentare l'esatto intento di un metodo.

È un anti-pattern comune o ci sono motivi significativi per non farlo?

Inoltre, la risposta sarebbe diversa se avessi usato eccezioni?

Answer 1

risposta solida

E 'assolutamente accettabile: il codice sorgente .NET avvolge condizioni.

Ad esempio, lo StringBuilder source code ha un metodo chiamato VerifyClassInvariant() da chiamare 18 volte. Il metodo controlla solo la correttezza.

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

Va bene alle affermazioni di gruppo o eccezioni in un metodo statico, piuttosto che scrivere esplicitamente?

Sì. Va bene.

... sembra andare contro l'idea che le condizioni preliminari dovrebbero aiutare a documentare l'esatto intento di un metodo.

Il nome del metodo che avvolge le assert o Exception dichiarazioni dovrebbe comunicare l'intento del wrapper. Inoltre, se il lettore desidera conoscere le specifiche, può visualizzare l'implementazione del metodo (a meno che non sia closed-source)

È considerata pratica buona o cattiva e perché?

E 'buona pratica per avvolgere una serie di correlati o comunemente riutilizzati asserts in un altro metodo, perché può sia migliorare la leggibilità e facilitare la manutenzione.

È un anti-pattern comune?

Al contrario, in realtà. Potresti vedere qualcosa di simile ed è effettivamente utile e consigliato perché comunica bene.

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

    // other conditions omitted 
} 

E 'una buona idea aggiungere ThenThrow alla fine del metodo in modo che il chiamante sa che stai lanciando. Altrimenti, potresti voler restituire un bool e lasciare che il chiamante decida cosa fare se la condizione non riesce.

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

    // other conditions omitted 
} 

Poi il codice chiamante può buttare:

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

Oppure, ecco another example from the .NET source che genera un'eccezione se alcune condizioni non riescono

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

(la ThrowHelper tiri solo eccezioni.)

ci sono motivi significativi per non farlo?

Quello che posso pensare è che se il nome del metodo non spiega quello che stai controllando, e non c'è un modo semplice per visualizzare il codice sorgente, allora probabilmente dovresti evitare le condizioni del wrapping.