Problem
I was asked to refactor a class, to make it adhere to SOLID principles, with testability and readability. I thought I had done a decent job, only my feedback was:
a) Unnecessary introduction of AccountExists() method [I thought it increased readability]
b) Breach of SOLID principles using “switch”, “new” keywords
c) Backward compatibility not maintained [I was told I should not change the method signature of the MakePayment method]
Here is my refactored code:
I’ve left out most of the interface definitions because they are obvious, all methods implemented are defined on the interface:
public class NewPaymentService : IPaymentService
{
private readonly IAccountDataStore _accountDataStore;
private readonly IPaymentValidatorFactory _paymentValidatorFactory;
public NewPaymentService(
IAccountDataStore accountDataStore,
IPaymentValidatorFactory paymentValidatorFactory)
{
_accountDataStore = accountDataStore;
_paymentValidatorFactory = paymentValidatorFactory;
}
public MakePaymentResult MakePayment(MakePaymentRequest request)
{
if (!_accountDataStore.AccountExists(request.DebtorAccountNumber))
{
return new MakePaymentResult()
{
Success = false,
FailureReason = "Account does not exist"
};
}
var account = _accountDataStore.GetAccount(request.DebtorAccountNumber);
var paymentValidator = _paymentValidatorFactory.GetPaymentValidator(request.PaymentScheme);
bool isValid = paymentValidator.ValidatePayment(request, account);
if (!isValid)
{
return new MakePaymentResult()
{
Success = false,
FailureReason = "Payment is invalid"
};
}
account.Balance -= request.Amount;
_accountDataStore.UpdateAccount(account);
return new MakePaymentResult()
{
Success = true
};
}
}
public class PaymentValidatorFactory : IPaymentValidatorFactory
{
public IPaymentValidator GetPaymentValidator(PaymentScheme paymentScheme)
{
switch (paymentScheme)
{
case PaymentScheme.Bacs:
return new BacsPaymentValidator();
case PaymentScheme.FasterPayments:
return new FasterPaymentValidator();
case PaymentScheme.Chaps:
return new ChapsPaymentValidator();
default:
throw new ArgumentException($"No payment validator available for {paymentScheme}");
}
}
}
public class FasterPaymentValidator : IPaymentValidator
{
public bool ValidatePayment(MakePaymentRequest request, Account account)
{
if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.FasterPayments))
{
return false;
}
if (account.Balance < request.Amount)
{
return false;
}
return true;
}
}
public class AccountDataStore : IAccountDataStore
{
public bool AccountExists(string accountNumber)
{
throw new System.NotImplementedException();
}
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 : IAccountDataStore
{
public bool AccountExists(string accountNumber)
{
throw new System.NotImplementedException();
}
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
}
}
Here is the old method I was asked to refactor:
public class OldPaymentService : 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;
}
}
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
}
}
I cannot understand how the feedback was so critical. AccountExists(), it may be an extra database call, but it is better than returning null, which the reader has to assume means that the account doesn’t exist.
I inverted the dependencies so that IAccountDataStore type could be defined in a configuration startup class that does the DI, rather than having to check the app settings in the method.
I can’t see how using ‘new’ to instantiate a DTO (MakePaymentResult) is a violation of SOLID, not using ‘new’ is meant to apply to behaviour implementations. And they said not to change the signature, how else can you return a MakePaymentResult?!
I used switch in a Factory class to get the right IPaymentValidator. That’s the only place it was used, and I thought a Factory class was exactly the kind of place where you’d convert an enum to an implementation, for using the strategy pattern.
Can you give me any feedback on the feedback? What have I missed so badly?
Solution
Overall I agree with you, not the review you’ve been given. I think the sole exception here is their feedback on your AccountExists introduction.
I cannot understand how the feedback was so critical. AccountExists(), it may be an extra database call, but it is better than returning null, which the reader has to assume means that the account doesn’t exist.
I get your reasoning but I don’t agree that the extra call was justified here. Good practice is a guideline, and sometimes you have to make reasonable compromises for ulterior reasons such as performance. Doubling the amount of database calls is a step in the wrong direction here.
While there is much to say about null and putting whether you do/don’t like it aside, a “return it if it exists” which returns null is generally considered to be a valid approach. If it is contextually clear that a null is an expected possible outcome, this isn’t something you have to particularly avoid.
You’re also not really improving the consumer’s handling of the code. In either case, their logic remains:
if(/* not exists */)
{
/* return failure */
}
You’ve changed how you confirm the existence, but you haven’t really changed how consumers will use your datastore.
As an alternative, you could change your approach to a bool TryGetAccount(string accountNumber, out Account account). This is more explicitly telling the consumer that there may not be an account (hence “try”), and while you’d still be returning null if there isn’t one, the consumer doesn’t really have to actively perform a null check anymore.
I also want to point out that an “entity exists” type of method is perfectly fine to implement when there are cases where you need to know if something exists but have no interest in fetching it afterwards anyway. In this case, it would be a single database call either way, and AccountExists would indeed be more descriptive than a GetAccount call with an explicit null check.
I inverted the dependencies so that IAccountDataStore type could be defined in a configuration startup class that does the DI, rather than having to check the app settings in the method.
Your approach is good, and for the right reasons. No further comment needed.
Tangential aside: “inverted dependencies” are something different. This is not your fault, as Dependency Inversion/DI is not a great name. “Dependency Inversion” is very easy to mistake for inverted dependencies. DI (i.e. Dependency Inversion) is also very easy to mistake for Dependency Injection (also labeled DI). While these two concepts are often used at the same time, they are different.
This is why a lot of people have started referring to Dependency Inversion (DI) as Inversion of Control (IoC), to avoid the ambiguity.
I can’t see how using ‘new’ to instantiate a DTO (MakePaymentResult) is a violation of SOLID, not using ‘new’ is meant to apply to behaviour implementations. And they said not to change the signature, how else can you return a MakePaymentResult?!
This seems to be a mistake on the reviewer’s part where they’ve misapplied the advice for avoiding new for a class’ dependencies (i.e. because of IoC) and ended up telling you to avoid new to instantiate a result object. They’re wrong, you’re right.
I’m also not sure what they mean with “not changing the signature” since the old version also returned a MakePaymentResult.
I used switch in a Factory class to get the right IPaymentValidator. That’s the only place it was used, and I thought a Factory class was exactly the kind of place where you’d convert an enum to an implementation, for using the strategy pattern.
Note: There are a few variations on factory implementations. I’m only focusing on the one you’re using here.
A factory is a good way to abstract the instantiation process, whether it is based on an enum or something else. It is not the only way, but it is the most straightforward way to abstract it.
I agree with your approach here, with one potential caveat: the enum is found in MakePaymentRequest request. This might suggest that you could’ve made the decision on what payment validator to use at an earlier stage (still using a factory), and then you could’ve injected the validator itself (instead of the factory) into the NewPaymentService instance. But this is highly contextual and I can’t definitively tell you what makes the most sense for your codebase.
One tiny thing I would’ve done differently here is to also introduce a secondary interface to account for the concrete types of payment validators, i.e.:
public interface IPaymentValidator { /* as is */ }
public interface IFasterPaymentValidator : IPaymentValidator {}
public class FasterPaymentValidator : IFasterPaymentValidator {}
public interface IBacsPaymentValidator : IPaymentValidator {}
public class BacsPaymentValidator : IBacsPaymentValidator {}
The reason for this is testability and mockability. This enables your test logic to also mock a specific kind of validator. This can be useful in cases where other services might use a specific validator directly instead of delegating the choice of what validator to use to the factory.
However, this is again contextual. If all your usages of payment validator always delegate this decision to the factory, then the IPaymentValidator could suffice here. I tend to err on the side of implementing the extra interface anyway so I can be prepared for the future, but YAGNI might apply.
AccountExists() doesn’t seem a good choice here, as the extra call to the database could be unnecessary to the current project since GetAccount might already covers that, so, if(account == null) would be enough.
the return new MakePaymentResult it is not a bad approach, but it would be preferred if just stick to the original approach, as it’s simple and easy to readjust with future updates
PaymentValidatorFactory and all its related concretes is unnecessary. As you’re adding new requirements to the actual class without knowing the actual code. So, instead of introducing new classes, and requirements , it would be a safer option to just work within the class and not going outside it.
So, if i’m in your shoes, I would do something like :
public class OldPaymentService : IPaymentService
{
public MakePaymentResult MakePayment(MakePaymentRequest request)
{
MakePaymentResult result = new MakePaymentResult()
{
Success = false
};
if(request == null) return result;
IAccountDataStore accountDataStore = GetAccountDataStore();
Account account = accountDataStore.GetAccount(request.DebtorAccountNumber);
if(account == null) return result;
if(IsAllowedPayment(request, account))
{
account.Balance -= request.Amount;
accountDataStore.UpdateAccount(account);
result.Success = true;
}
return result;
}
private bool IsAllowedPayment(MakePaymentRequest request, Account account)
{
if(request != null && account != null)
{
var isAllowed = account.AllowedPaymentSchemes.HasFlag(request.PaymentScheme);
switch (request.PaymentScheme)
{
case PaymentScheme.FasterPayments:
return isAllowed && account.Balance > request.Amount;
case PaymentScheme.Chaps:
return isAllowed && account.Status == AccountStatus.Live;
}
}
return false;
}
private IAccountDataStore GetAccountDataStore()
{
var dataStoreType = ConfigurationManager.AppSettings["DataStoreType"];
switch(dataStoreType)
{
case "Backup":
return new BackupAccountDataStore();
default:
return new AccountDataStore();
}
}
}
this would keep things intact and narrowed the changes to the given scope. So, it’s minor changes, and won’t break the actual code behavior.
Dude, don’t sweat it. SOLID is used just to diss on other people 🙂 it is an insider joke 😀 .
You can tell because SOLID has nothing to do with performance, memory usage or readability or anything You can somehow measure …