Liskov Substitution and SRP Principle violation - how best to structure this scenario?

Question

While learning SRP and LSP, I'm trying to improve the design of my code to comply best with both of these principles. I have an employee class that has a calculatePay method on it.

Firstly, I believe following OOP SOLID Principles, calculatePay() method should not be an employee objects responsibility. Common responsibilities of employees would be to performDuties(), takeLunchBreak(), clockIn() and clockOut() etc.

Am I right in thinking this way? That's why I feel calculatePay() should belong in some other class. Okay so that's my SRP insecurity.

Coming to LSP:

I have subclasses like accountants, salesman, and directors. These are all employees that get paid. How would I change this design to better support volunteers? Volunteers don't get paid.

public class Employee {

    private String name;
    private int salary;
    private boolean topPerformer;
    private int bonusAmount;


    public Employee(String name, int salary, boolean topPerformer, int bonusAmount) {
       // set fields etc..
    }

    // This method doesn't seem to belong here.
    public int calculatePay(){
        if(topPerformer)
            return salary+bonusAmount;
         else{
            return salary;
         }
    }
}

Answer 1

As long as calculatePay() is as simple as in your example, and as long as it does not need any parameters more than members from Employee, I would leave the method where it is. But when calculatePay() becomes more complex, and it needs information from outside the Employee class (like information from the employer, about taxes, actual date, etc.) I would refactor the calculation into a separate class like a "PaymentCalculator".

To your second question: assumed a volunteer is also an employee, then it is one with salary and bonus of zero. As long as the code which actually calls the calculatePay does not expect a payment of greater zero, I don't see a violation of the LSP.

Answer 2

I would use composition to model this problem.

There are more classes this way, but the classes are better segregated. A person has a role. A role has a salary. If the company changes the way that pays are calculated it requires changes in the PayCalculator. If the definition of a person is changed, it only requires change in the Person class. If a person is a volunteer, they get a salary of 0 in their CompanyRole. Note that the same person can then later be promoted to a genuine employee quite easily (no need to create a new Person).

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

This is just a starting point. Consider creating an overload for the CalculatePay method which calls the method passing false as a parameter (this could clean up the calling code a little). Also note that it is not the responsibility of the Person class to store whether the person is the top performer. That should be the responsibility of a different class altogether (after all, there should only ever be one top performer, which is why it's a parameter).

Answer 3

For your first question, Strategy Pattern might be best solution. Each concrete strategy would be related to concrete employee type. But you should have separate employee types if different employees have different behaviors.

For your second question, it is similar situation as I answered here : Is this a violation of the Liskov Substitution Principle?

If you don't want to return 0 as "paid", then introduce IsPaid property of Employee which is checked before the payment calculation and execution takes place.

Answer 4

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

Yes and no. That behavior should not be the responsibly of the employee, but not because of any programming or design principle. It is because no company in the world lets their employees calculate their own pay, so it isn't how your model is. Simple as that.

It isn't a question of following SOLID, it is a question of modeling your business domain as your business domain really is.

If you don't know where responsibility should lie you go back to your domain and keep looking until it is crystal clear and you don't even have to ask the question. This isn't a programming principle, this is just design. No keyboard required. You need to understand the model, understand who are the objects and understand what their role and behavior is. You need to understand that before you even sit down to write any code.

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

How does the real world business deal with volunteers? Figure that out and then just reflect that in your objects.

If you are ever in doubt about where responsibility should go or which object should do what, just go back to your business model. It should be blindingly clear from that. If it isn't then you need to leave the code and do some more design until you understand how the business manages these things.

While many businesses are not run at top efficiency, few are run in a complete mess. It is likely that your business has already figured out what a "volunteer" is. You may speak to a a director and they will say "Oh god no, we don't think of volunteers as employees at all, they are completely different" In which case you reflect that in your code. Or they may say that pay roll just treats volunteers as employees that get paid 0 each month. Again reflect that in your model.

If this is a programming assignment or something and you are not actually modelling a real business, but rather making up the business as you go along with the code, I would stop and start again modelling a business you know actually exists and that you have some experience with how it is run and structured.

Answer 5

If you want to modell your classes by the single responsibility principle (SRP), you have to make up your mind what these responsibilities are.

When you want to use your Employee class in a salary payment application, then the CalculatePayment method is perfectly right in that place.

The Employee represents an employed person which for sure has a name. Also the Employee class has the responsibility to do (trigger) the calculation of its payment, but it's not neccesary that it knows how. In fact you are limiting your design when you give Employee details on how to calculate the payment.

A better approach could be the following:

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

With this approach every Employee can have a very special payment strategy and you also get rid of the ugly if statement in your calculatePay method.

The base for AbstractPayment could look as following:

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

Now you have to implement a class for any concrete payment, eg.:

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

Instantiate your Employees something like:

var fixedPayment = new FixedPayment(100m);

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

For the volunteer you have two posibilities of implementation, depending on the aggreed behaviour of CalculatePayment for an volunteer is an error or not.

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

Now you can instantiate your volunteers like so:

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