0

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;
         }
    }
}
  • 7
    Favor composition over inheritance. – Telastyn Sep 7 '14 at 16:34
  • Yes. But an accountant is an employee and so is a Director. Can't go around that fact by just saying use composition.. – Tazo Man Sep 7 '14 at 16:35
  • 10
    sure you can. People have jobs. Jobs aren't people. – Telastyn Sep 7 '14 at 17:05
  • hmm.. I don't think I follow. Could you elaborate how this would be possible via composition? thanks in advance! – Tazo Man Sep 7 '14 at 17:12
  • 1
    @TazoMan: Telasty is suggesting to model a "Person" class and a "Job" or "JobRole" class. This will allow a Person to have different Jobs, or change the Job or JobRole at a later point in time, or to model a person which has different jobs for different employers. Nethertheless this approach just shifts the inheritance tree from the Employee to the Job, and it may solve a problem, but a different one than the one you have asked for. – Doc Brown Sep 7 '14 at 19:13
4

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.

  • It still seems a bit messy to me for leaving a $0 in the employee calculatePay() method. I don't want my API callers to have to check for greater than 0. This is also a learning exercise for me, so I want to be really strict with my OOP design. So what is the best alternative to returning 0 in the volunteer subclass? – Tazo Man Sep 7 '14 at 16:47
  • @TazoMan: to me dealing with 0 not as a special case is much more generic and clean than to implement any changed behaviour for volunteers. There is nothing messy about it, it is a straightforward solution. Your current calculatePay example works perfectly when salary and bonus are 0, and there is, for example, no need to provide a special overload in your Volunteer class. – Doc Brown Sep 7 '14 at 19:04
  • From an academic and learning standpoint - if we strictly comply with SOLID, how can this design conform better with regards to payments? – Tazo Man Sep 7 '14 at 20:52
  • @TazoMan: which part of my suggestions do you think are not following the SOLID principles? – Doc Brown Sep 8 '14 at 6:26
  • CalculatePayment. As an employee, its not my responsibility to calculate my own payment – Tazo Man Sep 8 '14 at 14:17
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).

  • Though I think this is a better model, it is not a good example of changing the original model from "inheritance" to "composition". If you map the original description to this new model straightforward, this would lead to different subclasses of "CompanyRole", like "DirectorRole", "AccountantRole", "SalesmanRole". – Doc Brown Sep 8 '14 at 6:35
1

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.

  • Euphoric: The link you posted seems quite juicy, but since I'm no language expert, learning java at the moment, could you explain in java, how I could apply the IsPaid flag in my design? Thank you! – Tazo Man Sep 14 '14 at 6:46
  • @TazoMan The point is to make caller of the Employee aware, that there is possibility of him not being paid and also make it clear that calling CalculatePayment when IsPaid is false is invalid operation. – Euphoric Sep 14 '14 at 13:16
1

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.

1

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.