Design – Single Responsibility Principle Violation

designdesign-patternsobject-oriented-designsingle-responsibility

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?

Best Answer

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.

Related Solutions

Design – Single Responsibility Principle Implementation

I've always considered the single responsbility principle more of a philosophy than principle.

Robert C. Martin (Uncle Bob) restates the Single Responsibility Principle (linked to PDF):

There should never be more than one reason for a class to change

Quoted from the reply you got your post.

When we try to appease the SPR, we focus on the implementation rather than on the purpose.

Major mistake.

My understanding of the SRP is this, when an addition blurs the responsibility of an implementation, the focal point should be reconsidered.

For me, it is all about the relationships between purposes.

Sure, a user has to be authenticated and authorized regarding actions to take - but the authentication and authorization should be separated from the user.

It's not like you are going to try to authorize/authenticate a non-user anyway?

And thus, you should consider splitting implementation when the responsibilities of a class outgrows it purpose. After all, design patterns have purposes too.

Take a look at GRASP when you get a chance.

Design – Struggling with the Single Responsibility Principle

Like the Pirate Code, the SRP is more of a guideline than a rule, and it's not even a particularly well-worded one. Most developers have accepted the redefinitions of Martin Fowler (in Refactoring) and Robert Martin (in Clean Code), suggesting that a class should only have one reason to change (as opposed to one responsibility).

It's a good, solid (excuse the pun) guideline, but it's almost as dangerous to get hung up on it as it is to ignore it.

If you can add a Post to a Tag and vice versa, you haven't broken the single-responsibility principle. Both still only have one reason to change -- if the structure of that object changes. Changing the structure of either one does not change the way it is added to the other, so you are not adding a new "responsibility" to it.

Your final decision should really be dictated by the functionality required on the front end. There will likely be a need to add a Tag to a Post at some point, so do something like the following:

// C-style-language pseudo-code
class Post {
    string _title;
    string _content;
    Date _date;
    List<Tag> _tags;

    Post(string title, string content) {
        _title = title;
        _content = content;
        _date = Now;
        _tags = new List<Tag>();
    }

    Tag[] getTags() {
        return _tags.toArray();
    }

    void addTag(Tag tag) {
        if (_tags.contains(tag)) {
            throw "Cannot add tag twice";
        }

        _tags.Add(tag);
        tag.referencePost(this);
    }

    // more stuff here, obviously
}

class Tag {
    string _name;
    List<Post> _posts;

    Tag(string name) {
        _name = name;
    }

    Post[] getPosts() {
        return _posts.toArray();
    }

    void referencePost(Post post) {
        if (!post.getTags().contains(this) || _posts.contains(post)) {
            throw "Only reference a post by calling Post.addTag()";
        }

        _posts.Add(post);
    }

    // more stuff here too
}

If, later, you find a need to add Posts to Tags as well, just add an addPost method to the Tag class and a referenceTag method to the Post class. Obviously, I have named them differently so that you don't accidentally cause a stack overflow by calling addTag from addPost and addPost from addTag.

Best Answer

Related Solutions

Design – Single Responsibility Principle Implementation

Design – Struggling with the Single Responsibility Principle

Related Topic