HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Side effects in functions that find boolean conditions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
conditionsbooleansideeffectsthatfindfunctions

Problem

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
{
private IFactory eom;
public CreateCampaign(IFactory 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")]

Solution

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.

Context

StackExchange Code Review Q#12036, answer score: 2

Revisions (0)

No revisions yet.