4
\$\begingroup\$

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<Campaign> AllCampaigns();

    //FIND
    Campaign FindCampaignBy(Expression<Func<Campaign, bool>> expression);
    CampaignDetails FindCampaignDetailsBy(Expression<Func<CampaignDetails, bool>> expression);

    //FILTER
    IList<Campaign> FilterCampaignBy(Expression<Func<Campaign, bool>> expression);
    IList<CampaignDetails> FilterCampaignDetailsBy(Expression<Func<CampaignDetails, bool>> 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 UpdateCampaignDetails(CampaignDetails campaignToUpdate)
    {
        _campaignDetailsRepository.Update(campaignToUpdate);
    }

Doing the same thing basically but with different entities.

Example of using method above:

    [HttpPost]
    public ActionResult Create(Campaign campaign)
    {
        if (ModelState.IsValid)
        {
            try
            {
                _campaignService.InsertCampaign(campaign);
                var campaignDetails = new CampaignDetails()
                {
                    CampaignId = campaign.CampaignId
                };

                _campaignService.InsertCampaignDetails(campaignDetails);
                _campaignService.Save();

                return RedirectToAction("Index");
            }
            catch (Exception ex)
            {
                ModelState.AddModelError(string.Empty, "Something went wrong. Message: " + ex.Message);
            }
        }

        return View(campaign);
    }
\$\endgroup\$
  • \$\begingroup\$ You have a design issue. Your ICampaignService is too low level. Post an example method that uses UpdateCampaignDetails(CampaignDetails) and we will have a better idea what can be done to clean up the interface. \$\endgroup\$ – abuzittin gillifirca Mar 20 '14 at 13:24
  • \$\begingroup\$ I can't help but wondering whether this "service" actually provides any additional value to the Entity Framework context? At best it's a leaky abstraction, at worst it's no abstraction at all, just a middle man. \$\endgroup\$ – MattDavey Mar 20 '14 at 13:29
  • \$\begingroup\$ also you probably should not use an object as id. I'd guess you actually have a long or BigInteger as Id, why not pass one? \$\endgroup\$ – Vogel612 Mar 20 '14 at 13:33
  • \$\begingroup\$ @abuzittingillifirca what do you mean low-level? And please see edit. \$\endgroup\$ – Jack Mar 20 '14 at 14:02
  • \$\begingroup\$ @MattDavey I have the same thoughts as you.. it's pretty much a pointless abstraction between the controller and the repository... But I was thinking if I want to create services that actually provide a useful service (i.e. sending out mailing campaigns) then I may as well have services that perform the CRUD operations too.. Or should I just have CRUD operations as Controller -> Repository. What if I need to perform a lot of logic before I create a new Campaign ? \$\endgroup\$ – Jack Mar 20 '14 at 14:06
5
\$\begingroup\$

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<Campaign> 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<Func<Campaign, bool>> expression as parameters) that are called from elsewhere.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for "This means that Campaign and CampaignDetails form an aggregate. And Campaign is the aggregate root." \$\endgroup\$ – MattDavey Mar 20 '14 at 16:52
  • \$\begingroup\$ Thank you very much you've made a lot of sense. I see now, instead of having UpdateCampaign in the service layer repeat the repository, you tailor it with logic, and then call the repository. This will be good for my EmailRecipient entity, because there will be different ways of adding an email (through .xls, .csv etc.) so the service will have different methods but end up calling a simple update query in the repository/DAL. \$\endgroup\$ – Jack Mar 20 '14 at 17:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.