Si considera questo codice ripetuto?

Tecnicamente, è codice ripetuto. Tuttavia, dove i tuoi colleghi si sbagliano, pensa che il codice ripetuto sia sempre cattivo. In questo caso, la chiarezza che la ripetizione aggiunge supera di gran lunga gli aspetti dannosi della ripetizione, come la creazione di più punti di errore. È improbabile che l'euristica del design venga portata agli estremi per produrre il miglior design. Devi capire le ragioni dietro l'euristica.

Ora, se tu avessi pagine di queste funzioni, ognuna con combinazioni di criteri leggermente diverse, allora la regola DRY diventa il fattore dominante. Il trucco consiste nel riconoscere il punto di equilibrio e il refactoring del codice quando cresce così.

In una nota a margine, volevo sottolineare che quando si passa ripetutamente lo stesso oggetto del primo argomento a una funzione statica, è un buon segno che probabilmente si adatterebbe meglio come metodo di istanza, cioè basketItems.isInBasket(groupID) .

No, non è un codice ripetuto; sta usando il polimorfismo per consentire ai chiamanti di utilizzare due diversi overload.

Se il codice fosse più lungo in ogni metodo, io rifatterò le parti comuni nel loro metodo, ma le istruzioni linq sono già sufficientemente concise (supponendo che tu usi Any() come suggerito da altri).

Non l'ho inserito in un compilatore, ma, idealmente, quando si esegue l'overloading, non si duplica il codice da un metodo all'altro.

Si codifica il metodo completo e ogni sovraccarico lo chiama, passando null dove non ha parametri, e si finisce con qualcosa del tipo:

/// <summary>
/// Is a group in the basket already?
/// </summary>
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID)
{
    //return BasketItems.Where(c => c.GroupID == GroupID).SingleOrDefault() != null;
    return isItemInBasket(BasketItems, GroupID, null)
}
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate)
{
    return BasketItems.Where(c => c.GroupID == GroupID && (ArtworkDate.HasValue ? c.ArtworkDate == ArtworkDate.Value : true)).SingleOrDefault() != null;
}

Quando si finisce con sempre più sovraccarichi, è sufficiente cambiare il metodo completo (anche se il metodo più complicato). E ciascuno dei tuoi altri metodi sono solo wrapper, rimuovendo la duplicazione.

Se non intendi aggiungere alcuna logica aggiuntiva a quei metodi, allora sì, stai duplicando il codice. Stai meglio con BasketItems.Any(c => /* insert condition here */) .

Tuttavia, se disponi di molti punti nel codice in cui lo utilizzi, puoi fare qualcosa del genere:

public bool IsInBasket(List<BasketItem> basketItems, Expression<Func<BasketItem, bool>> filter)
{
    return basketItems.Any(filter.Compile());
}

Supponendo che stai usando .net 4.0. Finisci con un solo metodo con un parametro facoltativo

public static bool IsItemInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate = null)
{
    var artWorkDate = ArtworkDate ?? DateTime.Now;
    return BasketItems.Any(c => c.GroupID == GroupID && c.ArtworkDate == artWorkDate);
}

È molto borderline. Non mi piacciono i test unitari che dovresti scrivere.

Preferirei vedere un metodo sul BasketItem che legge

public bool isInBasket(int GroupID, DateTime? ArtworkDate)
{
    return this.GroupID == GroupID &&
           this.ArtworkDate == ArtworkDate ?? this.ArtworkDate;
}

E poi

/// <summary>
/// Is a group in the basket already?
/// </summary>
public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID)
{
    return isItemInBasket(BasketItems, GroupID, null);
}

public static bool isItemInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate)
{
    return BasketItems.Any(c => c.isInBasket(GroupID, ArtworkDate)) != null;
}

Potrei andare oltre e mettere un'interfaccia su BasketItem in modo da poter inserire risposte false su questi test.

Tutto ciò che ho detto, non ti deluderebbe su una revisione del codice per il tuo codice, ti avvertirei che se diventasse più complicato sarebbe un problema.

Modifica: In realtà, rileggendolo, ho appena notato che dimostra il punto. Perché ora non ha senso. Questo mi costringe a riconsiderare la mia denominazione.

public bool isInGroup(int GroupID, DateTime? ArtworkDate)
{
    return this.GroupID == GroupID &&
           this.ArtworkDate == ArtworkDate ?? this.ArtworkDate;
}

E poi

/// <summary>
/// Is a group in the basket already? (do I even need this comment any more?)
/// </summary>
public static bool isGroupInBasket(List<BasketItem> BasketItems, int GroupID)
{
    return isItemInBasket(BasketItems, GroupID, null);
}

public static bool isGroupInBasket(List<BasketItem> BasketItems, int GroupID, DateTime? ArtworkDate)
{
    return BasketItems.Any(c => c.isInGroup(GroupID, ArtworkDate)) != null;
}

Ulteriore modifica: In realtà, più avanti percorriamo questa strada, più penso di vedere il problema. Tutti, incluso me stesso, chi sente che è duplicato sta reagendo al sovraccarico del metodo, che d'istinto dovrebbe essere un metodo che chiama un altro.

Ma in realtà, il secondo metodo sta facendo qualcosa di molto, molto diverso. E il fatto che nessuno di noi sia in grado di capire a cosa serve questo campo ArtworkDate suggerisce che forse il nome del secondo metodo dovrebbe essere cambiato. isGroupInBasketAnd ... Cosa?

Penso che tu veda il codice come non duplicato perché sai quanto sia diverso, ma tutti gli altri guardano, vedono lo stesso nome e pensano che sia solo un sovraccarico standard, un po 'di zucchero sintattico.