SEWP – Is the Single Responsibility Principle applicable to functions?

As it turns out, I don’t peruse the Programmers StackExchange quite as much as I initially thought.  However, after much ado, I’ve finally decided to target a post for my second StackExchange Writing Prompt. Here’s a relatively simple question in my eyes that shows how a simple problem could have multiple solutions.

I won’t directly quote the initial post, but I will point you to the link here:

http://programmers.stackexchange.com/questions/275646/is-the-single-responsibility-principle-applicable-to-functions

What we’re looking at is a CalculatePay() method that uses a switch statement to call another method depending on the employee type. The question here is how exactly this breaks the Single Responsibility Principle. It’s believed that a clear violation of the SRP is that there is more than one need for this method to change.
While I don’t intend to go too deeply into why this is incorrect and would rather talk about ambiguity in creating a better solution, I’d first like to talk a little bit about its incorrectness and violation of the SRP.  First off, this method acts as a routing mechanism for all unique types of payable employees.  Every time you need a new employee type (let’s say a contractor) we would need to write a new pay method and modify the CalculatePay() method to .  Furthermore, if our enum changes, we now have to come back and modify this method.

Writing code like this is a surprisingly common pitfall for the junior developer. I myself am guilty in the past of using switch() statements against enums to perform logic.  In a bind it is a quick and dirty method to handle that type of polymorphic logic and lends itself perfectly to the competent new developer who has mastered flow control.  It’s a prime example of code that works correctly but does not scale well.

So this of course begs the question: how could we design this better?  Is there a clear-cut way to make this objectively superior?  Is there only one true way to accomplish this? We don’t have enough information here to make a truly informed decision, but we can at least take a first stab at it.  Let’s rewind for a second and look at how we’re defining employees:

    public Money calculatePay(Employee e) throws InvalidEmployeeType {
        switch (e.type) {
            case COMMISSIONED:
                return calculateCommissionedPay(e);
            case HOURLY:
                return calculateHourlyPay(e);
            case SALARIED:
                return calculateSalariedPay(e);
            default:
                throw new InvalidEmployeeType(e.type);
        }
    }

Of course we have very little to work with which makes things complicated, but just looking at this implementation I can already think of six questions to ask that may lead you to an optimal solution?

  1. Are there any classes in the system that inherit from Employee?
  2. Does the Employee class expose the CalculatePay() method?
  3. Where are the other CalculatePay() methods held?
  4. Is there one predominant (AKA default) payment method?
  5. Are there employees who don’t get paid (IE unpaid interns)?
  6. Could two employees of the same type have different payment methods?
  7. (Bonus) Could there be a situation where an employee has multiple pay methods (IE salaried and commissioned)

Let’s knock the easy one out of the way first.  If you have two employees of the same type who can be paid differently, I would question your underlying decision-making in what warrants a derived classes vs what warrants a type property. For example, if you were breaking your employees down by Manager vs Analyst but you had Analysts who could be salaried or hourly, you might want to consider using a type parameter as there is no significant change in the way they’re processed by your software system based on those derived classes.

If you’re in a situation where you want a default implementation, it might make sense to have a Employee base class containing a CalculatePay() method with a default implementation that others could then override.  Alternatively, you could provide an abstract/virtual method with no implementation that the derived classes would then be forced to implement.

virtual

Assume for a moment that not all employees get paid in our unpaid intern situation.  Should the Employee class even house a CalculatePay() method?  Perhaps it may make more sense to have payable employees to implement an IPayable interface for which they could provide their own payable implementation.  This then of course begs the question as to how your employees are stored in the database, especially given that each payment type would likely need access to different data fields.  I’m on the fence about this type of implementation in practice.

interface

How about a scenario where employees can have multiple payment methods?  If we only had one such case (IE someone who is salaried with commission) I might be tempted to create an indirect subclass of employee that inherits from one of Employee’s subclasses.  If, however, we started running into multiple combinations of these (IE hourly and revenue sharing, salaried and revenue sharing, salaried and commissioned with revenue sharing) you might lean more towards something else entirely like the Decorator pattern, which would allow you to incrementally apply that logic:

indirect

Coming back to the point I intended to derive out of this question, we have now seen multiple potential implementations to solve a problem that appears quite simple on the surface but can encapsulate many complexities.  To say that one answer is right would be incorrect because we have multiple things to consider. I’m all for the principle of YAGNI, but at some level a business-aligned development team should be able to reasonably predict what potential cases may come down in the future.  Implementing something like a Decorator pattern to resolve item 7 would certainly be overkill in a situation that didn’t actively require it, however having the foresight to know that unpaid interns could very well be part of the system and having the foresight to choose the appropriate design up-front can save plenty of hours of re-work and reduce long-term complexity.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s