Liskov Sostituzione e violazione del principio SRP: come strutturare al meglio questo scenario?

Fintanto che calculatePay() è semplice come nel tuo esempio, e fintanto che non ha bisogno di alcun parametro più dei membri di Employee , lascerei il metodo dov'è. Ma quando calculatePay() diventa più complessa, e ha bisogno di informazioni al di fuori della classe Employee (come le informazioni dal datore di lavoro, sulle tasse, la data effettiva, ecc.) Rifarei il calcolo in una classe separata come "PaymentCalculator" .

Alla tua seconda domanda: supposto che un volontario sia anche un impiegato, quindi è uno con stipendio e bonus pari a zero. Finché il codice che effettivamente chiama calculatePay non prevede un pagamento di maggiore zero, non vedo una violazione dell'LSP.

Vorrei usare la composizione per modellare questo problema.

Ci sono più classi in questo modo, ma le classi sono meglio segregate. Una persona ha un ruolo. Un ruolo ha uno stipendio. Se la società cambia il modo in cui vengono calcolati i pagamenti, è necessario apportare modifiche al PayCalculator. Se la definizione di una persona viene cambiata, richiede solo un cambiamento nella classe Persona. Se una persona è un volontario, ottiene uno stipendio di 0 nel suo CompanyRole. Nota che la stessa persona può in seguito essere promossa a un vero impiegato abbastanza facilmente (non è necessario creare una nuova persona).

class Person
{
    string Name { get; private set; }
    CompanyRole Role { get; set; }

    public Person(string name)
    {
        this.Name = name;
    }
}

class CompanyRole
{
    Decimal Salary { get; set; }
    string Name { get; private set; }

    public CompanyRole(string roleName)
    {
        this.Name = roleName;
    }
}

static class PayCalculator
{
    private CompanyRole role;
    private Decimal bonus;

    Decimal CalculatePay(bool topPerformer)
    {
        return topPerformer ? role.Salary : role.Salary + bonus;
    }

    public PayCalculator(CompanyRole role, Decimal bonus)
    {
        this.role = role;
        this.bonus = bonus;
    }
}

// elsewhere in the application
public void PayRun(Person topPerformer)
{
    foreach (var person in Company)
    {
        var calculator = new PayCalculator(person.Role, bonusAmount);

        // this method is defined elsewhere
        PayPerson(person, calculator.CalculatePay(person == topPerformer);
    }
}

Questo è solo un punto di partenza. Considera la possibilità di creare un sovraccarico per il metodo CalculatePay che chiama il metodo che passa come parametro falso (questo potrebbe eliminare un po 'il codice chiamante). Si noti inoltre che non è responsabilità della classe Person memorizzare se la persona è il migliore. Questa dovrebbe essere la responsabilità di una classe diversa del tutto (dopotutto, dovrebbe esserci sempre una top performer, motivo per cui è un parametro).

Per la tua prima domanda, Strategy Pattern potrebbe essere la soluzione migliore. Ogni strategia concreta sarebbe correlata al tipo di impiegato concreto. Ma dovresti avere tipi di dipendenti distinti se diversi dipendenti hanno comportamenti diversi.

Per la tua seconda domanda, è una situazione simile alla quale ho risposto qui: Questa è una violazione del principio di sostituzione di Liskov?

Se non vuoi restituire 0 come "pagato", inserisci la proprietà IsPaid di Employee che viene controllata prima che il calcolo e l'esecuzione del pagamento avvengano.

Firstly, I believe following OOP SOLID Principles, calculatePay() method should not be an employee objects responsibility... Am I right in thinking this way?

Sì e no. Questo comportamento non dovrebbe essere responsabile del dipendente, ma non a causa di alcun principio di programmazione o di progettazione. È perché nessuna azienda nel mondo consente ai propri dipendenti di calcolare la propria retribuzione, quindi non è come è il tuo modello. Semplice come quello.

Non si tratta di seguire SOLID, si tratta di modellare il proprio dominio aziendale come il proprio dominio aziendale è .

Se non sai dov'è la responsabilità, torna al tuo dominio e continua a cercare finché non è chiaro e non devi nemmeno fare la domanda. Questo non è un principio di programmazione, questo è solo design. Nessuna tastiera richiesta. Devi capire il modello, capire chi sono gli oggetti e capire qual è il loro ruolo e comportamento. Devi capirlo prima ancora di sederti a scrivere qualsiasi codice.

How would I change this design to better support volunteers? Volunteers don't get paid.

In che modo il mondo reale si occupa di volontari? Dimostralo e poi rifletti solo quello nei tuoi oggetti.

Se hai dei dubbi su dove la responsabilità dovrebbe andare o su quale oggetto dovrebbe fare cosa, torna al tuo modello di business. Dovrebbe essere chiarissimo da quello. Se non è così, devi lasciare il codice e fare un po 'di design fino a quando non capisci come l'azienda gestisce queste cose.

Mentre molte aziende non sono gestite con la massima efficienza, poche vengono gestite in un disastro completo. È probabile che la tua azienda abbia già capito cos'è un "volontario". Potresti parlare con un regista e loro diranno "Oh dio no, non pensiamo affatto ai volontari come dipendenti, sono completamente diversi" Nel qual caso lo rifletti nel tuo codice. Oppure possono dire che la busta paga tratta semplicemente i volontari come dipendenti che vengono pagati 0 ogni mese. Ripeti di nuovo quello nel tuo modello.

Se si tratta di un incarico di programmazione o qualcosa del genere e non si sta veramente modellando un business reale, ma piuttosto di creare il business man mano che si procede con il codice, vorrei fermarmi e ricominciare a modellare un'attività che effettivamente esiste e che hai una certa esperienza su come è gestito e strutturato.

Se vuoi modellare le tue classi con il principio di responsabilità singola (SRP), devi decidere quali sono queste responsabilità.

Quando vuoi utilizzare la tua classe Employee in un'applicazione di pagamento salariale, il metodo CalculatePayment ha perfettamente ragione in quel luogo.

Il Employee rappresenta un impiegato che di sicuro ha un nome. Anche la classe Employee ha la responsabilità di eseguire (innescare) il calcolo del suo pagamento, ma non è necessario che sappia come. Di fatto, stai limitando il tuo design quando fornisci i dettagli di Employee su come calcolare il pagamento.

Un approccio migliore potrebbe essere il seguente:

public class Employee
{
    private readonly string name;
    private readonly AbstractPayment payment;

    public Employee(string name, AbstractPayment payment)
    {
        this.name = name;
        this.payment = payment;
    }

    public decimal CalculatePayment()
    {
        return payment.Calculate();
    }
}

Con questo approccio ogni Employee può avere una strategia di pagamento molto speciale e puoi anche eliminare la brutta dichiarazione if nel tuo metodo calculatePay .

La base per AbstractPayment potrebbe avere il seguente aspetto:

public abstract AbstractPayment
{
    public abstract decimal Calculate();
}

Ora devi implementare una classe per qualsiasi pagamento concreto, ad esempio:

public class FixedPayment : AbstractPayment
{
    private readonly decimal salary;

    public FixedPayment(decimal salary)
    {
        this.salary = salary;
    }

    public override decimal Calculate()
    {
        return salary;
    }
}

public class TopPerformerPayment : AbstractPayment
{
    private readonly AbstractPayment basePayment;
    private readonly decimal bonus;

    public TopPerformerPayment(AbstractPayment basePayment, decimal bonus)
    {
        this.basePayment= basePayment;
        this.bonus = bonus;
    }

    public override decimal Calculate()
    {
        return basePayment.Calculate() + bonus;
    }
}

Crea un'istanza del tuo Employees qualcosa come:

var fixedPayment = new FixedPayment(100m);

var regularEmployee = new Employee("Peter", fixedPayment);
var topEmployee = new Employee("Paul", new TopPerformerPayment(fixedPayment, 500m));

Per il volontario hai due possibilità di implementazione, a seconda del comportamento aggrappato di CalculatePayment per un volontario è un errore o meno.

public class VolunteerPaymentIsError : AbstractPayment
{
    public override decimal Calculate()
    {
        throw new PaymentNotAllowedException("A volunteer can not be paid!");
    }
}

public class VolunteerPaymentIsZero : AbstractPayment
{
    public override decimal Calculate()
    {
        return 0m;
    }
}

Ora puoi istanziare i tuoi volontari in questo modo:

var forbiddenPaymentVolunteer = new Employee("Bob", new VolunteerPaymentIsError());
var zerroSalaryVolunteer = new Employee("Bill", new VolunteerPaymentIsZero());