3
\$\begingroup\$

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?

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Who is "they"? Is this for work or school? \$\endgroup\$
    – Reinderien
    Apr 7, 2022 at 18:36
  • \$\begingroup\$ It is for work. \$\endgroup\$
    – zola25
    Apr 7, 2022 at 18:38
  • \$\begingroup\$ I don't think the AccountExists does improve readability - a check for null would be just as clear. The rest of the feedback seems... misguided. I mean, that's pretty much a textbook implementation of the factory pattern. I'm surprised you didn't get feedback about multiple returns being bad given the first code's extra complexity just to have one return. I don't see how you've broken backwards compact either - you've only added a property to the return type? \$\endgroup\$
    – RobH
    Apr 7, 2022 at 20:21
  • 1
    \$\begingroup\$ The original code is easier to read. \$\endgroup\$
    – CWallach
    Apr 8, 2022 at 2:33
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$
    – Mast
    Apr 8, 2022 at 16:52

3 Answers 3

2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ This is so subjective :D "As you're adding new requirements to the actual class without knowing the actual code" - that's what abstraction is. The way you have it here means that the payment service has multiple responsibilities and testing the validation logic in isolation is impossible. \$\endgroup\$
    – RobH
    Apr 8, 2022 at 10:08
  • 1
    \$\begingroup\$ @RobH from an experience point of view, working with what you have and leave the rest until you have it. That means, employers won't let you change everything and they expected you to get the job done within the deadline and keep your changes scoped to whatever you've been asked for. If you have some good approaches that will improve the overall project, then note that to the employer but don't include it in the current project unless they asked to. developer sees it coding, while employer sees it money. It's sucks but that's the reality. \$\endgroup\$
    – iSR5
    Apr 8, 2022 at 16:21
  • \$\begingroup\$ @iSR5 it if was a quick refactoring job, I would agree with your solution. But I was specifically asked to refactor using SOLID principles. Your code does not adhere. Just taking the Single Responsibility principle, your class has more than one reason to change - the validation logic could change for any of the payment schemes, or there could be a new type of IAccountDataStore that needs conditional access. So, while maybe 'quick and dirty with technical debt' is necessary sometimes, it doesn't apply for this task. \$\endgroup\$
    – zola25
    Apr 8, 2022 at 17:16
  • \$\begingroup\$ @zola25 I think you're missing the point, when you've asked to follow SOLID principles, it does not mean you should always apply them all on every task. You only have to apply what you see fit within the current context. They intentionally gave you only one method to work with to see how are you going to handle it without knowing the actual implementation and its dependencies. Believe me, your work is much better than the original, and it would be applicable only if you know what's happing behind the scene. \$\endgroup\$
    – iSR5
    Apr 8, 2022 at 19:01
  • \$\begingroup\$ @iSR5 - that's your experience. The point of SOLID is that they are a set of principles to help guide you to build maintainable software. The refactor in OP's post is less than an hour's work and allows for better testing which can achieve business benefits - e.g. preventing invalid payments directly saves a company money. It's your job as an engineer to sell the most effective solution. Your suggested refactor is purely cosmetic and is not an good use of time IMO. \$\endgroup\$
    – RobH
    Apr 11, 2022 at 6:55
-2
\$\begingroup\$

Dude, don't sweat it. SOLID is used just to diss on other people :) it is an insider joke :D .

You can tell because SOLID has nothing to do with performance, memory usage or readability or anything You can somehow measure ...

\$\endgroup\$
6
  • 2
    \$\begingroup\$ This answer is woefully misguided. The complexities and pushed deadlines due to the increased effort and likelihood of regressions during changes made to a bad practice codebase is very measurable. However, bad practice is often a matter of straws that eventually break the camel's back; meaning that you cannot directly relate a particular bad practice decision to solely be the cause of the delays and complexities that you encounter. This answer relies on the assumption that if you can't directly connect a specific cause and effect, that the effect somehow doesn't exist or cannot be addressed. \$\endgroup\$
    – Flater
    Apr 8, 2022 at 12:01
  • 1
    \$\begingroup\$ SOLID code means the code is scalable and can be extended in other ways such as adding new features. \$\endgroup\$
    – pacmaninbw
    Apr 8, 2022 at 14:43
  • \$\begingroup\$ @pacmaninbw is it possible to add features without SOLID? I wonder how people managed to create any software before ... \$\endgroup\$
    – user712092
    Apr 8, 2022 at 16:16
  • \$\begingroup\$ @Flater and definition of "bad practices" is "not doing SOLID", right? :D \$\endgroup\$
    – user712092
    Apr 8, 2022 at 16:18
  • 1
    \$\begingroup\$ @user712092: Bad practice is not defined as not doing SOLID. SOLID is a collection of some (definitely not all) of the most widely applicable and commonly agreed upon good practice guidelines. Generally speaking "bad" practice is a phantom name we give to a lack of following good practice guidelines in a scenario that would clearly benefit from doing so. \$\endgroup\$
    – Flater
    Apr 9, 2022 at 9:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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