Refactoring code to avoid anti-pattern

Question

I have a BusinessLayer project which has the following code. The domain object is FixedBankAccount (which implements IBankAccount).

The repository is made as a public property of the domain object and is made as an interface member. How to refactor it so that repository will not be an interface member?
The domain object (FixedBankAccount) makes use of the repository directly to store the data. Is this a violation of Single Responsibility Principle? How to correct it?

Note: The repository pattern is implemented using LINQ to SQL.

EDIT

Is the code given in the following a better approach? https://codereview.stackexchange.com/questions/13148/is-it-good-code-to-satisfy-single-responsibility-principle

CODE

public interface IBankAccount
{
    RepositoryLayer.IRepository<RepositoryLayer.BankAccount> AccountRepository { get; set; }
    int BankAccountID { get; set; }
    void FreezeAccount();
}

public class FixedBankAccount : IBankAccount
{
    private RepositoryLayer.IRepository<RepositoryLayer.BankAccount> accountRepository;
    public RepositoryLayer.IRepository<RepositoryLayer.BankAccount> AccountRepository
    {
        get
        {
            return accountRepository;
        }
        set
        {
            accountRepository = value;
        }
    }

    public int BankAccountID { get; set; }

    public void FreezeAccount()
    {
        ChangeAccountStatus();
    }

    private void SendEmail()
    {

    }

    private void ChangeAccountStatus()
    {
        RepositoryLayer.BankAccount bankAccEntity = new RepositoryLayer.BankAccount();
        bankAccEntity.BankAccountID = this.BankAccountID;

        accountRepository.UpdateChangesByAttach(bankAccEntity);
        bankAccEntity.Status = "Frozen";
        accountRepository.SubmitChanges();
    }
}

public class BankAccountService
{
    RepositoryLayer.IRepository<RepositoryLayer.BankAccount> accountRepository;
    ApplicationServiceForBank.IBankAccountFactory bankFactory;

    public BankAccountService(RepositoryLayer.IRepository<RepositoryLayer.BankAccount> repo, IBankAccountFactory bankFact)
    {
        accountRepository = repo;
        bankFactory = bankFact;
    }

    public void FreezeAllAccountsForUser(int userId)
    {
        IEnumerable<RepositoryLayer.BankAccount> accountsForUser = accountRepository.FindAll(p => p.BankUser.UserID == userId);
        foreach (RepositoryLayer.BankAccount repositroyAccount in accountsForUser)
        {
            DomainObjectsForBank.IBankAccount acc = null;
            acc = bankFactory.CreateAccount(repositroyAccount);
            if (acc != null)
            {
                acc.BankAccountID = repositroyAccount.BankAccountID;
                acc.accountRepository = this.accountRepository;
                acc.FreezeAccount();
            }
        }
    }
}

public interface IBankAccountFactory
{
     DomainObjectsForBank.IBankAccount CreateAccount(RepositoryLayer.BankAccount repositroyAccount);
}

public class MySimpleBankAccountFactory : IBankAccountFactory
{
    public DomainObjectsForBank.IBankAccount CreateAccount(RepositoryLayer.BankAccount repositroyAccount)
    {
        DomainObjectsForBank.IBankAccount acc = null;

        if (String.Equals(repositroyAccount.AccountType, "Fixed"))
        {
            acc = new DomainObjectsForBank.FixedBankAccount();
        }

        if (String.Equals(repositroyAccount.AccountType, "Savings"))
        {
            acc = new DomainObjectsForBank.SavingsBankAccount();
        }

        return acc;
    }
}

READING:

FWIW in most dependency injection scenarios, the Repository would have to be made public on the *class* for DI to work (i.e. at face value violating encapsulation), however, dependencies (like Repository) would NOT need be added to the Interface. All access to the class instance will be through the interface and hence the fact that the Repository class property is public is a non-event. However, since you are using factory method for instantiation, public dependencies aren't needed. — StuartLC, Jun 27 '12 at 10:39

score 3 · Answer 1 · answered Jun 27 '12 at 12:42

I wouldn't say that it's an anti-pattern since an anti-pattern is supposed to be a pattern in the first place (a recognizable, widespread way of doing things) and I don't know of any "Repository-in-the-Domain-object" pattern.

However, it's certainly bad practice IMO because your BankAccount domain object mixes 3 responsibilities :

Its natural and legitimate responsibility as a Domain object to freeze itself and change its status.
The responsibility to update and submit changes to a persistent store (using the accountRepository).
The responsibility to decide how a message should be sent (in that case, an email) and send it.

As a result, your Domain object is tightly coupled to too many things, making it rigid and fragile. It could change and possibly break for too many reasons.

So no anti-pattern but a violation of the Single Responsibility Principle for sure.

The last 2 responsibilities should be moved to separate objects. Submitting changes rather belongs in an object that manages the business transaction (Unit of Work) and is aware of the right time to end the transaction and flush things. The second one could be placed in an EmailService in the Infrastructure layer. Ideally, the object that does the global Freeze operation shouldn't be aware of the message delivery mechanism (by mail or something else) but should be injected with it instead, which would allow for more flexibility.

Thanks. Could you please provide some demo code too for your explanation? — LCJ, Jun 27 '12 at 14:21

score 3 · Answer 2 · answered Jun 27 '12 at 18:43

Refactoring this code so that the repository is not an interface member is easy enough. The repository is a dependency of the implementation, not the interface - inject it into your concrete class, and remove it from the IBankAccount.

public class FixedBankAccount : IBankAccount
{
    public FixedBankAccount(RepositoryLayer.IRepository<RepositoryLayer.BankAccount> accountRepository)
    {
        this.accountRepository = accountRepository;
    }

    private readonly RepositoryLayer.IRepository<RepositoryLayer.BankAccount> accountRepository;

    public int BankAccountID { get; set; }
    public void FreezeAccount()
    {
         ChangeAccountStatus();
    }

    private void SendEmail()
    {
    }

    private void ChangeAccountStatus()
    {
        RepositoryLayer.BankAccount bankAccEntity = new RepositoryLayer.BankAccount();
        bankAccEntity.BankAccountID = this.BankAccountID;

        accountRepository.UpdateChangesByAttach(bankAccEntity);
        bankAccEntity.Status = "Frozen";
        accountRepository.SubmitChanges();
    }

}

In regards to the second question...

Yes, the domain object is violating SRP by being aware of your persistence code. This may or may not be a problem, however; many frameworks mix these responsibilities for great effect - for example, the Active Record pattern. It does make unit testing a little more interesting, in that it requires you to mock your IRepository.

If you choose to have a more persistent-ignorant domain, you would probably best do so by implementing the Unit of Work pattern. Loaded/edited/deleted instances get registered in the Unit of Work, which is responsible for persisting changes at the end of the transaction. The unit of work is responsible for your change tracking.

How this is setup depends on the type of application you're creating and the tools you're using. I believe if working with Entity Framework, for example, you may be able to use the DataContext as your unit of work. (Does Linq-to-SQL have the notion of a DataContext as well?)

Here's an example of the Unit of Work pattern with Entity Framework 4.

Thanks. I am using LINQ 2 SQL. It has DataContext. Can you please explain how to change the code (in LINQ 2 SQL) to make it a Unit of Work ? — LCJ, Jun 28 '12 at 06:47
@Lijo Sorry, I don't use Linq 2 SQL and don't know it well enough to be able to comment. Best of luck. — Remi Despres-Smyth, Jun 28 '12 at 11:39
@Lijo, here's a pretty good introduction on Unit of Work by Jeremy Miller. If I were in your shoes, I'd start by trying to get a good understanding of Unit of Work generally, and then focus on how you can implement it in your Linq-to-SQL project. http://msdn.microsoft.com/en-us/magazine/dd882510.aspx — Remi Despres-Smyth, Jun 28 '12 at 14:26

score 1 · Answer 3 · answered Jun 30 '12 at 12:23

1

The solution of Remi is much better, but a better solution IMO would be this:

1- Don't inject anything to domain objects: you do not need to inject anything into your domain entities.Not services. Not repositories. Nothing. Just pure domain model goodness

2- Let Service layer direct repositories to do SubmitChanges,... but be aware that service layer should be thin & domain objects should not be anemic

answered Jun 30 '12 at 12:23

raminxtar

11
1

My preference is also to work with a more "pure" domain - certainly a persistence-ignorant one. – Remi Despres-Smyth Jul 03 '12 at 12:37

score 1 · Answer 4 · answered Jul 01 '12 at 10:06

Interfaces have nothing to do directly with the single responsibility principle. You can't completely separate data access code from business logic—they have to communicate at some point! What you want to do is minimize (but do not avoid) where this happens. Make sure your database schema is logical not physical (i.e., based on predicates and not tables and columns) and that the implementation-based code (e.g., database management system connectivity drivers) are only in one place—the class responsible for talking to the database. Each entity should be represented by one class. That's it.

Refactoring code to avoid anti-pattern

4 Answers4

Linked