Un Enum sta cambiando il comportamento di un'entità cattiva?

Alcuni puristi potrebbero obiettare che questo è intrinsecamente un cattivo progetto. La mia opinione è meno rigida su questo.

Come molte cose in programmazione, c'è una soglia di ciò che è accettabile e ciò che non lo è. Esploriamo gli estremi:

inaccettabile :

Un'entità che si comporta in modo completamente diverso in tutte le sue proprietà / metodi, in base all'enum.

In questo caso, hai praticamente unito diverse entità separate in una singola, e utilizzi l'enum per selezionare fondamentalmente quale comportamento dell'entità vuoi utilizzare.

accettabile :

Un'entità che si comporta allo stesso modo indipendentemente dall'enumerazione, ad eccezione di una piccola percentuale delle sue proprietà / metodi.

In questo caso, le entità continuano a rappresentare lo stesso ruolo e l'enum ha solo un impatto minore sull'entità nel suo insieme.

Un buon esempio qui sarebbe l'appelazione di una persona in base al sesso:

public string FormalName
{
    get
    {
        switch(this.Gender)
        {
            case Gender.Male:
                return $"Mr {this.LastName}";
            case Gender.Female:
                if(this.IsMarried)
                    return $"Mrs {this.LastName}";
                else
                    return $"Ms {this.LastName}";
        }
    }
}

Questo corrisponde al conto della tua domanda, è un enume che modifica il comportamento della sua entità. Tuttavia, il cambiamento stesso è solo una piccola parte dello scopo dell'entità, il che lo rende trascurabile in termini di necessità di refactoring.

Per esempi più astratti, alcuni puristi potrebbero sostenere l'implementazione / composizione dell'ereditarietà / interfaccia. Ho alcuni colleghi che sosterrebbero sicuramente questo punto di vista.
Ma lo trovo troppo ingegnerizzato e quindi prima di tutto valutare se un'astrazione sia realmente garantita o meno. In questo caso, non penso che sia giustificato.

Quindi dove cadi nello spettro?

È un po 'difficile da valutare, dal momento che hai omesso parte della tua classe che non è influenzata dall'enumerazione.

Per quanto posso vedere, l'enum decide solo la validazione dell'entità. Questo è accettabile, purché non si utilizzino essenzialmente campi / valori completamente diversi tra due enumerazioni diverse.

• Se enum A richiede FieldA e FieldB; e enum B richiede FieldC e FieldD; quindi sono davvero due entità completamente separate.
• Se entrambe le enumerazioni richiedono 10 campi e differiscono solo su un particolare campo richiesto, allora questo è un comportamento accettabile (IMO).

Chiediti se una qualsiasi delle alternative sarebbe meglio qui:

• La distinzione tra le enumerazioni è abbastanza grande da giustificare entità separate?
• Una delle enumerazioni è una versione "più specifica" di un'altra enumerazione (più generalizzata)? Se è così, forse starai meglio con due entità che derivano l'una dall'altra.
• ...

Revisione codice

Tuttavia, mi chiedo a proposito dello scopo del metodo stesso:

public void AddBusinesses(param Business[] businesses)
{
    if(MembershipType == MembershipType.TypeD && businesses.Length != 2)
        throw new InvalidOperationException("Membership Type D require 2 businesses");
    else if(businesses.Length != 1)
        throw new InvalidOperationException($"MembershipType {MembershipType} require 1 business");

    //...
}

Il metodo sembra presupporre che quando viene chiamata AddBusinesses() ; che è fornito con la lista completa delle imprese.
Lo deduco in base al fatto che tu lanci immediatamente un'eccezione se non sono state fornite abbastanza imprese. Se _businesses contiene già un elemento, e io uso il metodo per aggiungere un secondo elemento, non ci dovrebbe essere un errore di convalida per TypeD , tuttavia ne verrà comunque generato uno.

Tuttavia, generalmente mi aspetto che anche i metodi Add() possano essere concatenati. Supponiamo che io faccia questo:

var myMembership = new Membership() { MembershipType = MembershipType.TypeD };

foreach(var business in myListOfFiveBusinesses)
{
    if(business.NeedsToBeAdded)
        myMembership.AddBusinesses(business);
}

Questo codice fallirebbe, perché si genererebbe un'eccezione quando aggiungo la prima azienda. Non aspetti mai di vedere se avessi aggiunto più imprese!

La parte strana è che il resto del codice sembra funzionare con l'idea che questo metodo possa essere chiamato più volte:

    foreach(var business in businesses)
    {
        _businesses.Add(business);
    }

Tieni presente che questo codice aggiunge le attività commerciali a un esistente elenco di _businesses .

Se fosse il caso che un elenco preesistente venisse sovrascritto, mi aspetterei un codice diverso qui, ad es.

_businesses = businesses; //out with the old list!

Sembra che il tuo attuale metodo AddBusinesses() venga utilizzato in modo incoerente. La convalida presuppone che i parametri specificati sovrascrivano qualsiasi elenco preesistente; ma la logica successiva presuppone che i parametri dati siano aggiunti a un elenco preesistente.

Nella mia idea, potrebbero essere applicati tutti e 5 i SOLID. Prima di iniziare con DDD (questo è un alto livello di visualizzazioni). Penso che possiamo iniziare con un design di base con SOLID e acquisire maggiore esperienza nella progettazione dello scopo dei modelli. E il loro non è il modo migliore. Trova l'implementazione più adatta. Ed ecco i miei interessi.

public interface IBusiness
{
}

public enum MembershipType
{
    TypeA,
    TypeB,
    TypeC,
    TypeD,
}

public interface IMemberShip
{
   MembershipType Type { get; }
}

public abstract class MemberShip : IMemberShip
{
    public abstract MembershipType Type { get; }
}

public abstract class SingleBusinessMemberShip : MemberShip
{
    protected SingleBusinessMemberShip(IBusiness business)
    {
        Business = business ?? throw new ArgumentNullException(nameof(business));
    }

    public IBusiness Business { get; }
}

public abstract class MultipleBusinessesMemberShip : MemberShip
{
    protected MultipleBusinessesMemberShip(IList<IBusiness> businesses)
    {
        Businesses = businesses ??  throw new ArgumentNullException(nameof(businesses));
    }

    public IList<IBusiness> Businesses { get; }
}

e le sottoclassi di appartenenza dovrebbero richiedere business in base alle necessità, a seconda che si tratti di una singola azienda o di più aziende

public class TypeAMemberShip1 : SingleBusinessMemberShip
{
    public TypeAMemberShip1(IBusiness business) : base(business)
    {
    }

    public override MembershipType Type => MembershipType.TypeA;
}

public class TypeAMemberShip2 : SingleBusinessMemberShip
{
    public TypeAMemberShip2(IBusiness business) : base(business)
    {
    }

    public override MembershipType Type => MembershipType.TypeA;
}

public class TypeDMemberShip1 : MultipleBusinessesMemberShip
{
    public TypeDMemberShip1(IList<IBusiness> businesses) : base(businesses)
    {
    }

    public override MembershipType Type => MembershipType.TypeD;
}

public class TypeDMemberShip2 : MultipleBusinessesMemberShip
{
    public TypeDMemberShip2(IList<IBusiness> businesses) : base(businesses)
    {
    }

    public override MembershipType Type => MembershipType.TypeD;
}

Un'applicazione sana del principio di apertura / chiusura è ciò che è necessario qui

the MembershipType enum changes the behaviour of the Membership entity

Non se MembershipType è incapsulato nell'astrazione appropriata. Crea una classe Businesses , che naturalmente saprà che è il minimo e amp; numero massimo di aziende. Quindi il Membership solo sa - viene detto dall'oggetto business - se Businesses è pieno o no, indipendentemente da quale MembershipType sia.

Il modello MembershipType manca qualcosa?

È difficile immaginare che i tuoi clienti abbiano questi termini che indicano solo il numero di attività consentite. Ci deve essere dell'altro. Se è così una classe Businesses è un buon inizio per rendere il concetto estendibile nel codice.

Di conseguenza Membership diventa un'API completamente coerente per tutto MembershipType ish. L'esposizione di dettagli banali, come visto nel metodo AddBusinesses , rovina l'estensibilità e uccide una buona API.

Costruzione

Utilizza un factory come suggerito da @BobDalgleish che inietterà il Businesses personalizzato tramite un costruttore di Membership . E usa MembershipType per dire alla fabbrica cosa costruire.

Se il valore MembershipType stesso diventa parte di quella classe è un problema di progettazione. Ma non essere un'astrazione che perde, rendendola pubblica, o forza (!), Membership o qualsiasi Businesses client, fai ciò che Businesses dovrebbe fare per se stesso. Fatti gli affari tuoi.