Should I create two seperate Services and then combine them or have one large?

Basically, I have two separate tables, Campaign and CampaignDetails. They relate via CampaignID. I want a service that can handle both at the same time, as they're closely related and when one gets updated- so will the other.

At the moment I'm combining the two but it's starting to get a bit hard to look at

public interface ICampaignService
{
    //INSERT
    void InsertCampaign(Campaign campaign);
    void InsertCampaignDetails(CampaignDetails campaign);

    //UPDATE
    void UpdateCampaign(Campaign campaign);
    void UpdateCampaignDetails(CampaignDetails campaign);

    //DELETE
    void DeleteCampaign(Campaign campaign);
    void DeleteCampaign(object id);
    void DeleteCampaignDetails(CampaignDetails campaign);
    void DeleteCampaignDetails(object id);

    //ALL
    IList AllCampaigns();

    //FIND
    Campaign FindCampaignBy(Expression> expression);
    CampaignDetails FindCampaignDetailsBy(Expression> expression);

    //FILTER
    IList FilterCampaignBy(Expression> expression);
    IList FilterCampaignDetailsBy(Expression> expression);

    //GET
    Campaign GetCampaignByID(object id);
    CampaignDetails GetCampaignDetailsByID(object id);

    Campaign GetLatestCampaign();
    Campaign GetMostRecentSentCampaign();

    //SAVE
    void Save();
}

This is just my interface, so the implementation is obviously a lot more.. And it seems a lot of similar code.

I came across this article http://bobcravens.com/2010/09/the-repository-pattern-part-2/ and I quite like the DataService that wraps all the individual services. But is it bad practise to say have one variable DataService mainService = new DataService(repo1, repo2, repo3, repo4) that can the access every service?

So I should I split it into CampaignService and CampaignDetailsService or keep it as one?

EDIT

Example of update method

```
public void UpdateCampaign(Campaign campaignToUpdate)
{
_campaignRepository.Update(campaignToUpdate);
}

public void UpdateCampaignDe

Overview of your service looks like this. I've put T-SQL commands that execute in each methods comment.

interface ICampaignService
{
    //INSERT = INSERT INTO CAMPAIGN....
    void InsertX( ... );

    //UPDATE = UPDATE CAMPAIGN....
    void UpdateX( ... );

    //DELETE = DELETE FROM CAMPAIGN
    void DeleteX( ... );

    //ALL = SELECT * FROM CAMPAIGN
    IList AllCampaigns();

    //FIND = SELECT * FROM CAMPAIGN WHERE ...
    //FILTER
    //GET
    Campaign FindX( ... );
    Campaign GetCampaignByID(object id);

    //SAVE = COMMIT TRANSACTION;
    void Save();
}

And you yourself say:


I have two separate tables, Campaign and CampaignDetails. They relate via CampaignID.

The problem is your service is not just a thin layer over Entity Framework it still is a thin layer over SQL. In fact even your action names mirror database commands.

The service interface must be a must be the interface to your business logic layer.
And repositories be your gateway to the data access layer.

Let's look at the Create(Campaign campaign) action for a second:

Create(Campaign campaign)
{
// BOILER PLATE IGNORED
    _campaignService.InsertCampaign(campaign);
    var campaignDetails = new CampaignDetails()
    {
        CampaignId = campaign.CampaignId
    };

    _campaignService.InsertCampaignDetails(campaignDetails);
    _campaignService.Save();
// BOILER PLATE IGNORED
}

When your user pressed the submit button one time, did he intend 4 different things?

Create(Campaign campaign)
{
// BOILER PLATE IGNORED
    _campaignService.CreateCampaign(campaign);
// BOILER PLATE IGNORED
}

It is clear that this more closely captures user's intent.

void CreateCampaign(Campaign campaign)
{
    _campaignRepository.InsertCampaign(campaign);
    var campaignDetails = new CampaignDetails()
    {
        CampaignId = campaign.CampaignId
    };

    _campaignRepository.InsertCampaignDetails(campaignDetails);
    _campaignRepository.Save();
}

As you do the above transformation -in other words above refactoring- for each action there will be unused methods on the service interface. Remove them and your service interface will be cleaner.


I want a service that can handle both at the same time, as they're closely related and when one gets updated- so will the other.

This means that Campaign and CampaignDetails form an aggregate. And Campaign is the aggregate root. (I'm just giving you the standard terminology, so you can more easily look up the situation on the Internet.)

In situation where you have closely related [entities] and [such that] when one gets updated- so will the other, or in database-speak a master table and one ore more detail tables, you service should not contain direct references to the detail entities, or in other words you service should only deal with the aggregate root, in this particular case Campaign. It only makes sense that ICampaignService only deals with Campaigns directly.

I had asked for the UpdateCampaignDetails example usage so that I can give a more concrete example of this but we will have to make do with the made up example below:

Suppose we are talking about sales campaigns here and one thing that is stored in CampaignDetails is a discount rate for the Campaign. After the above refactoring to your service and controller instead of UpdateCampaignDetails your service will have methods like below:

void ChangeDiscountRate(Campaign campaign, decimal newDiscountRate)

or

void ChangeDiscountRate(Guid CampaignId, decimal newDiscountRate)

whose implementation would be something like:

var campaignDetailsToUpdate = _campaignDetailsRepository.GetById(CampaignId);
campaignDetailsToUpdate.DiscountRate = newDiscountRate;
_campaignDetailsRepository.Update(campaignDetailsToUpdate);
_campaignDetailsRepository.Save();

You need not stop there. Ideally after a next round of refactorings (Including defining One-to-One and one-to-many relations where necessary) your service methods will read like below:

var campaignToUpdate = _campaignRepository.GetById(CampaignId);
campaignToUpdate.ChangeDiscountRate(newDiscountRate);
_campaignRepository.Update(campaignToUpdate );

or more generally:

var aggregateRoot = repository.GetById(id);
aggregateRoot.ModifyThisEntityAndItsDetails(...);
repository.Update(aggregateRoot);

The other thing to do would be separate the methods that update campaigns (hint those methods that return void, who are also the methods that are called from [HttpPost] controller methods from search methods (hint they are the method that return SomeCollection and take Expression> expression as parameters) that are called from elsewhere.

StackExchange Code Review Q#44854, answer score: 5