Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I had code that violated the Single Responsibility Principle in a question on Stack Overflow.

In order to overcome that problem, I changed the code as follows.

  1. Is this a good practice in order to overcome the problem?
  2. Is this a pattern?
  3. Is there a better way?
  4. Does it satisfy the Unit Of Work pattern using LINQ-to-SQL?

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

    public class FixedBankAccount : IBankAccount
    {
        public int BankAccountID { get; set; }
        public string AccountStatus { get; set; }

        public void FreezeAccount()
        {
            //ChangeAccountStatus();
            AccountStatus = "Frozen";
        }

        //private void ChangeAccountStatus()
        //{
        //    AccountStatus = "Frozen";
        //    RepositoryLayer.BankAccount repositoryBankAccEntity = new RepositoryLayer.BankAccount();
        //    repositoryBankAccEntity.BankAccountID = this.BankAccountID;
        //    accountRepository.UpdateChangesByAttach(repositoryBankAccEntity);
        //    repositoryBankAccEntity.Status = "Frozen";
        //    accountRepository.SubmitChanges();
        //}

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

using System.Collections.Generic;
using System;

namespace ApplicationServiceForBank
{
    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 oneOfRepositroyAccounts in accountsForUser)
            {
                DomainObjectsForBank.IBankAccount domainBankAccountObj = bankFactory.CreateAccount(oneOfRepositroyAccounts);
                if (domainBankAccountObj != null)
                {
                    domainBankAccountObj.BankAccountID = oneOfRepositroyAccounts.BankAccountID;
                    domainBankAccountObj.FreezeAccount();

                    this.accountRepository.UpdateChangesByAttach(oneOfRepositroyAccounts);
                    oneOfRepositroyAccounts.Status = domainBankAccountObj.AccountStatus;
                    this.accountRepository.SubmitChanges();
                }
            }
        }
    }

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

    public class MySimpleBankAccountFactory : IBankAccountFactory
    {
        //Is it correct to accept repository inside factory?
        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;
        }
    }
}
share|improve this question

1 Answer 1

I'm not sure about some of your questions, but a few ideas I have are:

  1. Could you make the the account status and even account types an enumeration? I tend to try and err on the use of enumerations over string literals. I believe the latest version of Linq to SQL supports this?
  2. Why do you need to expose the repository on your IBankAccount interface. Why would the interface even care about a repository? I think this is better being supplied to any concrete classes via it's constructor? It would also make unit testing easier I would think.
  3. I personally wouldn't pass in the whole BankAccount object into the factory method. It only needs the account type so I would pass in that. Probably I would look at using a switch or if else in that factory method as well and throwing an exception if you can't create it (depends on your requirements of this method).

Just a few thoughts.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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