1

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;
         }
    }
}
gnat
  • 21,442
  • 29
  • 112
  • 288
Tazo Man
  • 159
  • 1
  • 5
  • 7
    [Favor composition over inheritance](http://en.wikipedia.org/wiki/Composition_over_inheritance). – Telastyn Sep 07 '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 07 '14 at 16:35
  • 10
    sure you can. People _have_ jobs. Jobs aren't people. – Telastyn Sep 07 '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 07 '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 07 '14 at 19:13
  • @docbrown - I would argue that how to slice the code so that change becomes easier is a very significant problem to address. That after all is the entire point of LSP/SRP. – Telastyn Sep 08 '14 at 15:01
  • What are you building? What software system is this Employee class for? – Winston Ewert Sep 08 '14 at 15:26
  • Myself. Self learning accountant – Tazo Man Sep 08 '14 at 15:34
  • @Telastyn: I agree the Person/Role model is most probably better, nevertheless the solution you are suggesting is *not* demonstrating how to "favor composition over inheritance". It demonstrates how to use composition for better separation of concerns, but it is not a solution which eliminates the need for inheritance. See my comment below Stephen's answer. – Doc Brown Sep 08 '14 at 15:48
  • Doc Brown, would you recommend a type hierarchy for employees? Yea composition is better but hierarchy for where it makes sense is also important isn't it? – Tazo Man Sep 08 '14 at 21:17
  • The hierarchy could also be applied on the `Role`/`Job` level instead. I would also argue that a `Volunteer` is not an `Employee` as they do not get paid. – Zymus Apr 13 '16 at 19:45

5 Answers5

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.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • 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 07 '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 07 '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 07 '14 at 20:52
  • @TazoMan: which part of my suggestions do you think are not following the SOLID principles? – Doc Brown Sep 08 '14 at 06:26
  • CalculatePayment. As an employee, its not my responsibility to calculate my own payment – Tazo Man Sep 08 '14 at 14:17
  • @TazoMan: reponsibilities (in OO class design) depend on the data needed for a task, and the complexity of a task. In your very simple model above, the reponsibility for calculating the payment can be kept in the `Employee` class, but only because all data needed is available in the member variables. For most real-world scenarios this model is too simple, payment calculation will need additional data, leading to a separate PaymentCalculator class. There are some good examples for more complex models in the other answers, so I am not going to suggest another one. – Doc Brown Sep 08 '14 at 16:08
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).

Stephen
  • 8,800
  • 3
  • 30
  • 43
  • 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 08 '14 at 06: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.

Community
  • 1
Euphoric
  • 36,735
  • 6
  • 78
  • 110
  • 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 06: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.

Cormac Mulhall
  • 5,032
  • 2
  • 19
  • 19
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());
abto
  • 181
  • 1
  • 1
  • 6