Single Responsibility Principle Violation?

Question

I recently got into a debate with another developer regarding the below class:

public class GroupBillingPayment
{
    public void Save(IGroupBillingPayment model)
    {
        if (model == null || UserInfo.UserID == 0)
        {
            throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
        }

        Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
        Mapper.Map(model, groupBillingPayment);
        ServiceManager.GroupBilling.IsBillAlreadyCancelled(groupBillingPayment.GroupBillingID, THROW_ERROR);
        groupBillingPayment.UpdatedBy = UserInfo.UserID;
        groupBillingPayment.UpdatedOn = DateTime.Now;
        RepositoryManager.GroupBillingPaymentRepository.Update(groupBillingPayment, false);
        UpdateGroupBilling([Parameters])
    }
}

I believe UpdateGroupBilling should not be called inside the save method as it violates the single responsibility principle. But, he says that each time a payment is made, the billing should be updated. Hence this is the correct approach.

My question, is SRP being violated here? If yes, how can we better refactor it so that both our criteria are met?

Answer 1

I would look at it this way:

A method should either call other methods (in same or other objects) which makes it a composed method or do "primitive calculations" (same level of abstraction).

The responsibility of a composed method is "call other methods".

So if your Save method does some "primitive calculations" itself (eg. checks return values), the SPR might be violated. If this "primitive calculations" lives in another method called by your Save method SRP is not violated.

---

The updated version of the Save method does not follow the single abstraction layer principle. Since this is the more important problem you should extract that to a new method.

This will convert Save into a composed method. As written, the responsibility of a composed method is "call other methods". Therefore calling UpdateGroupBilling([Parameters]) here is not a violation of the SRP, but a business case decision.

Answer 2

Single responsibility can be interpreted as function/class should have only one reason to change.

So your current Save method will violate that principle, because it should be changed by more then one reason.
1. Save logic changed
2. If you decide to update something else in addition to updating billing group

You can refactor it by introducing SaveModel method which will be responsible only for saving. And introducing another method which combine all needed operations based on your requirements. So you end up with two methods

public void SaveModel(IGroupBillingPayment model)
{
    // only saves model
}

public void Save(IGroupBillingPayment model)
{
    SaveModel(model);
    UpdateGroupBilling([Parameters]);
}

Where SaveModel method will have responsibility for saving model to the database and one reason to change - if saving "logic" will change.

Save method now have responsibility to call all required methods for full "saving" process, and have one reason to change - if amount of required method will change.

I think validation can be moved outside of SaveModel too.

Answer 3

Single Responsibility is IMHO a not easy to nail down concept.

A simple rule of thumb is:

When I have to explain, what a method/class does, and have to use the word »and«, it is an indicator, that something smelly may be going on.

On the one hand, it is only an indicator and on the other hand it works better reverse: If two things are going on, you can not avoid using the word »and« and since you are doing two things, you violate the SRP.

But, does that mean on the other hand, if you do more than one thing, you are violating SRP? No. Because otherwise you were limited to trivial code and trivial problems to solve. You would hurt yourself if your were too strict in the interpretation.

Another perspective on SRP is: one level of abstraction. As long as you are dealing with one level of abstraction, you are mostly fine.

What does all that mean for your question:

I believe UpdateGroupBilling should not be called inside the save method as it violates the single responsibility principle. But, he says that each time a payment is made, the billing should be updated. Hence this is the correct approach.

To decide, whether this is a violation of SRP, it is necessarry to know, what is going on in the save()-Method. If the method is -as the name- suggests responsible to persist the model to the database, a call to UpdateGroupBilling is IMHO a violation of SRP, since you are extending the context of »saving a payment«. You would paraphrase it with »I am saving the payment and update the group billing«, which is - as I said earlier - a (possible) indicator of violation of SRP.

On the other hand, if the method is describing a "payment-recipe" -i.e. what steps are to take in the process- and decide: everytime a payment is finished, it has to be saved (dealt with in another place) and then the group billings have to be updated (dealt with in another place), that would be no violation of SRP, since you aren't leaving the abstraction of the "recipe": you distinguish between what to do (in the recipe) and where it is done (in the according class/method/module). But if that is what your save()-method does (describing which steps are to take) you should rename it.

Without further context, it is hard to say anything concrete.

Edit: you updated your inital post. I would say, that this method violates the SRP and should be refactored. The fetching of the data should be factored out (it should be a parameter of this method). The addition of data (updateBy/On) should be done elsewhere. The saving should be done elsewhere. Then it would be OK to leave UpdateGroupBilling([Parameters]) as is.

Answer 4

Hard to be sure without full context but I think your intuition is correct. My personal favorite SRP indicator is whether you're going to know exactly where to go and change something to modify a feature months later.

A class defining a payment type is not where I would expect to go to redefine who does the paying or how the paying is done. I would expect it to be one of many payment types that simply provide critical details that some other part of the app uses to initiate, conduct, or roll back/cancel a transaction, gathering those details through a uniform interface.

It also looks like a recipe for a more general DRY principle violation if you're going to have multiple types each sorting out their own transactions using a lot of the same methods. SOLID doesn't always feel 100% relevant to primarily JavaScript developer (I suspect a lot of it's explained badly though) but DRY is the gold standard in general programming methodology IMO. Any time I come across an idea that feels like an extension of DRY, I give it consideration. SRP is one of those. If everybody stays on topic, you're less likely to be tempted to repeat yourself.

Answer 5

If there's conceptually only one way to UpdateGroupBilling(...), and it only ever happens in that place, then it's probably fine where it is. But the question is related to the concept, not to temporal circumstances, i.e. "for the time being we only do it here."

If either isn't the case, then one way to refactor it would be to use Publish/Subscribe and notify whenever the payment is saved, and have the billing subscribe to that event and update the relevant entries. This is good especially if you need to have several types of billings that need to be updated. Another method would be to think of Billing as a strategy pattern, i.e. pick one and use it, or accept it as well as a parameter. Or, if billing doesn't always happen, you can add it as a decorator, etc, etc. But again, this does depend on your conceptual model and how you want to think about it, so you'd have to figure it out.

One important thing to consider, though, is error handling. If billing fails, what happens with the previous operations?

Answer 6

I think this design is violating the SRP, but it's really easy to fix and to still do everything else is needed.

Think of it about messages and about what's happening in this method. It should be saving the GroupBillingPayment, but there's nothing wrong if GroupBillingPayment notifies classes that it's been saved. Especially if it's implementing a pattern that's explicitely exposing that behavior.

You could use the Observer pattern.

Here's an example of how it'd work in your case :

public interface Subject {

    public void register(Observer observer);
    public void unregister(Observer observer);

    public void notifyObservers();      
}

public class GroupBillingPayment implements Subject {

    private List<Observer> observers;
    public void Save(IGroupBillingPayment model)
    {
        if (model == null || UserInfo.UserID == 0)
        {
            throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
        }

        Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
        Mapper.Map(model, groupBillingPayment);
        ServiceManager.GroupBilling.IsBillAlreadyCancelled(groupBillingPayment.GroupBillingID, THROW_ERROR);
        groupBillingPayment.UpdatedBy = UserInfo.UserID;
        groupBillingPayment.UpdatedOn = DateTime.Now;

        RepositoryManager.GroupBillingPaymentRepository.Update(groupBillingPayment, false);
        //UpdateGroupBilling([Parameters]) The Observer will have this responsability instead now
        notifyObservers();
    }

    public void notifyObservers() {
        for (Observer obj : observers) {
            obj.update();
        }
    }
}

public interface Observer {     
    public void update();

    public void setSubject(Subject sub);
}

Now all you have to do is create one or more Observers that'll be bound to GroupBillingPayment and thus get notified whenever it gets saved. It keeps its own dependencies, doesn't know what notified so it doesn't depend on these at all.

Answer 7

You want to achieve X. Unless X is quite trivial, you write a method that will do everything to achieve X, call the method "achieveX", and call that method. The single responsibility of "achieveX" is to do everything to achieve X. "achieveX" shouldn't do other things.

If X is complex, then many actions may be needed to achieve X. The set of actions needed may change over time, so when that happens, you change the method achieveX, but if everything goes well, you can still just call it.

In your example, the method "Save"'s responsibility is not to save the bill and update the group billing (two responsibilities), its responsibility is to take all the necessary actions to make the bill permanently recorded. One responsibility. Maybe you named it badly.

Answer 8

If I understand your point I'd have the payment to raise an event such as "Payment made" and the class handling the billing subscribing it. In this way the payment handler knows nothing about the billing.