What's the best way to separate concerns for this code?

Question

In a previous question one of the comments from Dr. Herbie on the accepted answer was that my method was performing two responsibilities..that of changing data and saving data.

What I'm trying to figure out is the best way to separate these concerns in my situation.

Carrying on with my example of having a Policy object which is retrieved via NHibernate....

The way I'm currently setting the policy to inactive is as follows:

Policy policy = new Policy();
policy.Status = Active;

policyManager.Inactivate(policy);

//method in PolicyManager which has data access and update responsibility
public void Inactivate(Policy policy)
{
    policy.Status = Inactive;
    Update(policy);
}

If I were to separate the responsibility of data access and data update what would be the best way to go about it?

Is it better to have the PolicyManager (which acts as the gateway to the dao) manage the state of the Policy object:

Policy policy = new Policy();
policy.Status = Active;

policyManager.Inactivate(policy);
policyManager.Update(policy);

//method in PolicyManager
public void Inactivate(Policy policy)
{
    policy.Status = Inactive;
}

Or to have the Policy object maintain it's own state and then use the manager class to save the information to the database:

Policy policy = new Policy();
policy.Status = Active;

policy.Inactivate();

policyManager.Update(policy);

//method in Policy
public void Inactivate()
{
    this.Status = Inactive;
}

Answer 1

What I would do:

• Create a repository which saves and retrieves Policies. (PolicyRepository)

• If you have complex logic that must be performed to activate / deactivate a policy, you could create a Service for that. If that service needs access to the database, then you can pass a PolicyRepository to it, if necessary. If no complex logic is involved, and activating / deactivating a policy is just a matter of setting a flag to false or true, or if only members of the policy class are involved, then why is 'Activated' not a simple property of the Policy class which you can set to false / true ? I would only create a service, if other objects are involved, or if DB access is required to activate or deactivate a policy.

Answer 2

If the status is part of the state of the Policy class, then the Policy should also have the Inactivate method -- that's just basic encapsulation. Entangling multiple classes in a single responsibility is at least as bad as giving a single class multiple responsibilities.

Alternatively, the status could be considered metadata about the Policy, belonging not to the Policy but to the PolicyManager. In that case, though, the Policy shouldn't know its own status at all.

Answer 3

I would definitely go with the 3rd option for the reasons you mentioned:

the Policy object maintain it's own state and then use the manager class to save the information to the database

Also take a look at the Repository Pattern. It might substitute your PolicyManager.

Answer 4

As a continuation of my original comment :) ... Currently your best bet is the third option, but if things get more complex you could go with the second, while adding facade methods to perform pre-specified sequences:

Policy policy = new Policy();

policy.Status = Active;

policyManager.InactivateAndUpdate(policy);


//methods in PolicyManager
public void Inactivate(Policy policy)
{
    // possibly complex checks and validations might be put there in the future? ...
    policy.Status = Inactive;
}

public void InactivateAndUpdate(Policy policy)
{
    Inactivate(policy);
    Update(policy);
}

The InactivateAndUpdate is a kind of facade method, which is just there to make the calling code a little neater, while still allowing the methods doing the actual work to be separate concerns (kind of breaks single responsibility for methods, but sometimes you just have to be pragmatic!). I deliberately name such methods in the style X*and*Y to make them stand out as doing two things.

The InactivateAndUpdate method then frees you up to start implementing strategy patterns or splitting out the actual implementation methods as command objects for dynamic processing or whatever other architecture might become feasible in the future.