I am trying to refactor below code applying design patterns and trying to write tests. I have identified below code smells in this please suggest me if I am wrong or if anything I am missing.
public class AccountDataStore
{
public Account GetAccount(string accountNumber)
{
// Access database to retrieve account, code removed for brevity
return new Account();
}
public void UpdateAccount(Account account)
{
// Update account in database, code removed for brevity
}
}
public class BackupAccountDataStore
{
public Account GetAccount(string accountNumber)
{
// Access backup data base to retrieve account, code removed for brevity
return new Account();
}
public void UpdateAccount(Account account)
{
// Update account in backup database, code removed for brevity
}
}
public class PaymentService : IPaymentService
{
public MakePaymentResult MakePayment(MakePaymentRequest request)
{
var dataStoreType = ConfigurationManager.AppSettings["DataStoreType"];
Account account = null;
if (dataStoreType == "Backup")
{
var accountDataStore = new BackupAccountDataStore();
account = accountDataStore.GetAccount(request.DebtorAccountNumber);
}
else
{
var accountDataStore = new AccountDataStore();
account = accountDataStore.GetAccount(request.DebtorAccountNumber);
}
var result = new MakePaymentResult();
switch (request.PaymentScheme)
{
case PaymentScheme.Bacs:
if (account == null)
{
result.Success = false;
}
else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs))
{
result.Success = false;
}
break;
case PaymentScheme.FasterPayments:
if (account == null)
{
result.Success = false;
}
else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.FasterPayments))
{
result.Success = false;
}
else if (account.Balance < request.Amount)
{
result.Success = false;
}
break;
case PaymentScheme.Chaps:
if (account == null)
{
result.Success = false;
}
else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Chaps))
{
result.Success = false;
}
else if (account.Status != AccountStatus.Live)
{
result.Success = false;
}
break;
}
if (result.Success)
{
account.Balance -= request.Amount;
if (dataStoreType == "Backup")
{
var accountDataStore = new BackupAccountDataStore();
accountDataStore.UpdateAccount(account);
}
else
{
var accountDataStore = new AccountDataStore();
accountDataStore.UpdateAccount(account);
}
}
return result;
}
}
First change: I have extracted configuration and created a folder cofig and then created an IConfig interface and then an class implementing it, so I can more config settings in future if needed.
public interface IConfiguration
{
string DataStoreType { get; }
}
public class Configuration : IConfiguration
{
string IConfiguration.DataStoreType => ConfigurationManager.AppSettings["DataStoreType"];
}
Second Change: BackupAccountDataStore and AcccountDataStore are doing same thing AddAccount and Update so I have created an Interface so that these classes can implement it DRY don't repeat yourself
public interface IAccountDataStore
{
Account GetAccount(string accountNumber);
void UpdateAccount(Account account);
}
Third change: Too many switch statements removing them and implementing polymorphism, I have added classes instead of using switch statements
public class PaymentSchemeBacs:MakePaymentResult
{
public override bool Success( Account account)
{
if (account == null)
{
return false;
}
if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs))
{
return false;
}
return true;
}
}
Please kindly suggest me if anything needs to be change with in this code.
Aucun commentaire:
Enregistrer un commentaire