3
\$\begingroup\$

Here's an example of a logic structure I've refactored into something more complex on many occasions because I feel like it's wrong to have a side effect in a function that determines a yes/no answer to a question.

If I were to break this into multiple functions I would add to the main logic to call another function that handles the found conflict.

What is you opinion of logic like this? Should it be refactored?

(also I welcome any/all code critiques)

namespace DirectAgents.SynchService.Commands
{
    public class CreateCampaign : Command<Campaign, bool>
    {
        private IFactory<DADatabase> eom;
        public CreateCampaign(IFactory<DADatabase> eomDatabase)
        {
            this.eom = eomDatabase;
        }
        public override bool Execute(Campaign input)
        {
            bool result = false;
            using (var db = eom.Create())
            {
                if (this.CanAdd(input))
                {
                    db.Campaigns.AddObject(input);
                    Log("Saving new {0} [{1}].", input.GetType().Name, input.campaign_name);
                    db.SaveChanges();
                    result = true;
                }
            }
            return result;
        }
        private bool CanAdd(Campaign campaign)
        {
            using (var db = eom.Create())
            {
                var conflictingID = from c in db.Campaigns
                                    where (c.pid == campaign.pid) && (c.campaign_name == campaign.campaign_name)
                                    select c.pid;

                bool conflictExists = (conflictingID.FirstOrDefault() == default(int));
                if (conflictExists)
                {
                    this.CampaignConflicts.Add(new CampaignConflict {
                        Campaign = campaign,
                        ConflictingId = conflictingID.First()
                    });
                }
                return (!conflictExists);
            }
        }
        [Dependency("CampaignConflicts")]
        public ICollection<CampaignConflict> CampaignConflicts { get; set; }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Yes, I agree in that I think a side effect in a Yes/No method is not desirable. \$\endgroup\$ – dreza May 24 '12 at 22:48
2
\$\begingroup\$

I would personally separate the method as like Mark said it's having side effects as well as doing more than one thing.

Based on your code provided maybe something like this. Each method hopefully does one thing and the Execute uses these as required:

namespace DirectAgents.SynchService.Commands
{
    public class CreateCampaign : Command<Campaign, bool>
    {
        private IFactory<DADatabase> eom;
        public CreateCampaign(IFactory<DADatabase> eomDatabase)
        {
            this.eom = eomDatabase;
        }
        public override bool Execute(Campaign campaign)
        {
            bool result = true; // works unless told otherwise                
            int conflictingID = GetConflictingID(campaign);

            if(ConflictExists(conflictingID))
            {
                AddConflict(campaign, conflictingID);

                // a conflict exists so we failed to execute this operation
                result = false;
            }
            else
            {
                CommitChanges(campaign);                  
            }

            return result;
        }

        private void CommitChanges(Campaign campaign)
        {
            using (var db = eom.Create())
            {
                db.Campaigns.AddObject(campaign);
                Log("Saving new {0} [{1}].", campaign.GetType().Name, campaign.campaign_name);
                db.SaveChanges();                    
            }
        }

        private void AddConflict(Campaign campaign,int conflictingID)
        {
            this.CampaignConflicts.Add(new CampaignConflict
                    {
                        Campaign = campaign,
                        ConflictingId = conflictingID
                    });
        }

        private bool ConflictExists(int conflictID)
        {
            return conflictID == default(int);
        }

        private int GetConflictingID(Campaign campaign)
        {
            using (var db = eom.Create())
            {
                var conflictingID = from c in db.Campaigns
                                    where (c.pid == campaign.pid) && (c.campaign_name == campaign.campaign_name)
                                    select c.pid;

                return conflictingID.FirstOrDefault();
            }
        }

        [Dependency("CampaignConflicts")]
        public ICollection<CampaignConflict> CampaignConflicts { get; set; }
    }
} 

One thing I was tossing up was whether to switch the order of the if statements in the Execute method to have the normal flow go though the commitchanges method. Not really sure though.

Something like:

public override bool Execute(Campaign campaign)
        {
            bool result = true; // works unless told otherwise                
            int conflictingID = GetConflictingID(campaign);

            if(ConflictDoesNotExist(conflictingID))
            {
                CommitChanges(campaign);   
            }
            else
            {
                AddConflict(campaign, conflictingID);

                // a conflict exists so we failed to execute this operation
                result = false;               
            }

            return result;
        }
\$\endgroup\$
2
  • \$\begingroup\$ what do you think about bool ConflictExists(out int conflictingID)? \$\endgroup\$ – Aaron Anodide May 25 '12 at 22:25
  • \$\begingroup\$ @AaronAnodide I personally probably wouldn't do do that, doesn't mean it's not an ok idea. My main reason is it's conveying that the method is doing more than one thing. Checking a conflict exists and getting that conflicting ID? Others might have an opinion on this though. \$\endgroup\$ – dreza May 25 '12 at 22:36
2
\$\begingroup\$

The method name CanAdd implies that it won't have side effects. Therefore you could mistake the method in the future as a read-only method, while it isn't. Also because the relationship between checking whether it can be added to list A and adding it to another list is not strongly related, I would advice to make CanAdd a method that doesn't make any changes.

A better place to add the campaign to the conflicting list would be in execute or in a separate method called from execute.

\$\endgroup\$

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.