Una lunga lista di parametri controlla un anti-pattern?

Quando devi controllare le condizioni che non sono regole aziendali, è lì che diventa sospetto e un po 'puzzolente. Cerco di evitare quelli dove è possibile.

Alcuni dei tuoi test sembrano sospetti:

 if (linkType.BaseType != typeof(Link))

Questo sembra un controllo che dovrebbe essere fatto dal sistema di tipi. vale a dire una restrizione di tipo e idealmente la firma della propria funzione dovrebbe, per quanto possibile, accettare solo parametri del tipo corretto. Come farlo dipende da ulteriori dettagli su ciò che stai facendo.

 if (A.Equals(B) || B.Equals(A))

Perché senti il bisogno di controllarlo in entrambi i modi? Se A.quals (B)! = B.equals (A) ti trovi in un mondo di dolore.

 if (A == null || B == null || linkType == null)

Non è possibile stabilire quale parametro sia nullo dall'eccezione risultante. Viene eseguito anche dopo gli altri controlli, il che significa che probabilmente hai già causato un'eccezione nel tentativo di dereferenziare un parametro nullo.

Evito i controlli nulli in generale. In genere, non sono così utili perché il codice in questione finirà per generare un'eccezione dopo l'intervento sul null comunque e non ho guadagnato nulla spargendo assegni nulli in tutto il mio codice. Se si insiste su un controllo Null, suggerisco di estrarlo in una funzione di utilità.

Non penso che quello che hai fatto qui sia così male, anche se potrebbe essere spostato in una procedura separata per mantenere le cose più ordinate:

public void LinkGenerators(Generator A, Generator B, Type linkType)
{
     //simplest way to refactor is to just not catch the exception that's
     // thrown by the validation function.
     validateLinkGeneratorArgs(A, B, linkType);
     .....
     //Actually make the link after making sure all the arguments are valid.
}

private void validateLinkGeneratorArgs(Generator A, Generator B, Type linkType)
{
     if (linkType.BaseType != typeof(Link))
     {
        throw new ArgumentException("linkType is not a valid link type");
     }
     if (linkAlreadyExistsFor(A, B))
     {
        throw new InvalidOperationException("Link for A and B already exists");
     }
     if (A.Equals(B) || B.Equals(A))
     {
        throw new InvalidOperationException("A and B cannot be the same generator");
     }
     if (A == null || B == null || linkType == null)
     {
        throw new ArgumentException("Cannot pass a null argument");
     }
 }

Is a long list of parameter checks an anti-pattern?

Letteralmente no. Non ho mai sentito parlare di un "anti-pattern" che dice che molti parametri di controllo sono una brutta cosa. Anche il controllo dei parametri implementato come una sequenza di istruzioni if non è un anti-pattern.

Tuttavia, questo necessariamente significa che quello che stai facendo è giusto.

Il controllo degli errori in apertura in un metodo API presenta pro e contro:

• Sul lato "pro", si raccolgono problemi (errori di programmazione o problemi negli "input") e * arly. Questo (in genere) facilita la diagnosi dei problemi e prima che gli errori possano causare danni (o sprecare risorse).

• Sul lato "con", se il controllo degli errori è eccessivo o nel posto sbagliato, può essere dispendioso o inefficace.

Prendi questo frammento di codice per esempio:

 if (A == null || B == null || linkType == null)
 {
    throw new ArgumentException("Cannot pass a null argument");
 }

Ci sono due problemi con questo:

• Lo stai facendo troppo tardi. Se qualcuno di questi è null , allora già hai attivato un'eccezione.
• I test sono comunque ridondanti, poiché il CLR avrà già implicitamente testato per null nei precedenti controlli dei parametri. E le eccezioni risultanti sono altrettanto efficaci a fini diagnostici ... a condizione che tu abbia il codice sorgente e una traccia dello stack.

O questo:

 if (A.Equals(B) || B.Equals(A))
 {
    throw new InvalidOperationException("A and B cannot be the same");
 }

• È veramente importante che A e B non siano uguali? (Oppure potrebbe essere un errore innocuo, o forse anche qualcosa di utile?)

• È veramente necessario per testare le modalità di sia di uguaglianza? (Non puoi supporre che il metodo Equals sia stato implementato per essere riflessivo?)

Infine, c'è il problema più grande se i controlli siano (anche) ripetitivi. Dai suoni di esso, non sono nel tuo esempio. Ma in generale, potrebbero essere.

In sintesi. Nessun anti-pattern, ma è comunque necessario pensare su quanto sia consigliabile controllare gli errori e su come implementarlo al meglio.

if (linkType.BaseType != typeof(Link))
     {
        throw new ArgumentException("linkType is not a valid link type");
     }

dovrebbe essere

checkArgument(linkType.BaseType != typeof(Link),
    "linkType is not a valid link type");

In Java, c'è una libreria con una classe Precondizioni per questo (nessuna idea su c #, ma è facile da scrivere).

In questo modo ottieni codice più breve ed è pulito cosa è cosa. Per inciso, può anche essere più veloce man mano che il metodo si accorcia lasciando maggiori possibilità di inlining (almeno nella JVM). Tirare fuori tutti gli assegni in un unico metodo ... forse.

In generale, la convalida dell'input è buona (fallire presto). Per i metodi non pubblici, considera invece gli asseriti.

Most of the LinkGenerator functions consists of verifying that the parameters are good.

Questo è un odore di codice ... sembra che dovresti cercare alternative, come garantire alcune condizioni in modo che questo metodo possa essere semplificato. Ma questo non è sempre possibile.