4
\$\begingroup\$

The code that I am working with below is a payment service and MakePaymentRequest request takes in two account numbers, a receiving and a sending account, payment method type, an amount and a payment date.

I need to refactor the code in order to make it more aligned to the SOLID principles, however there are areas of SOLID that I may not fully there with yet and therefore am requesting help to refactor this.

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:
                BacsResults(account, result);
                break;
            case PaymentScheme.FasterPayments:
                FasterPaymentsResults(request, account, result);
                break;
            case PaymentScheme.Chaps:
                ChapsResults(account, result);
                break;
        }

        if (!result.Success) return result;
        account.Balance -= request.Amount;

        UpdateAccountDatastore(dataStoreType, account);

        return result;
    }

Other methods that contribute to this are as follows:

Update Account Datastore

private static void ChapsResults(Account account, MakePaymentResult result)
    {
        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;
        }
    }

Updating the BacsResults

private static void BacsResults(Account account, MakePaymentResult result)
    {
        if (account == null)
        {
            result.Success = false;
        }
        else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs))
        {
            result.Success = false;
        }
    }

Updating the FasterPayments

private static void FasterPaymentsResults(MakePaymentRequest request, Account account, MakePaymentResult result)
    {
        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;
        }
    }
\$\endgroup\$
  • \$\begingroup\$ ok, slow, start with a proper title and an explanation what your code is doing, then we'll see what can be done. currently it's completely unclear what you are asking. See: How do I ask a good question?. You've already asked one question and it was ok, do it like the last time but a little bit better :) \$\endgroup\$ – t3chb0t Jun 18 '17 at 7:47
  • \$\begingroup\$ @t3chb0t I have adjusted the title, and given more information, if thats not enough can you let me know what else is missing or is still unclear so that I can tidy this up more. \$\endgroup\$ – user127535 Jun 18 '17 at 7:53
  • \$\begingroup\$ The title could still be better. It's a payment service so maybe write it the title PaymentService for something - replace something with a more meaningfull description of course :) or anything else that roughly says what the code is doing, not the question you have. \$\endgroup\$ – t3chb0t Jun 18 '17 at 7:58
4
\$\begingroup\$

There will be many areas in your application where you will be doing checks based on the PaymentScheme enum. One thing you can do is turn that into a class and the behavior associated with that enum can go into the class. I notice that BacsResults and ChapsResults have a common interface so you can do this:

public abstract class PaymentSchemeResult
{
    public abstract MakePaymentResult GetResult(Account account);
}

public class PaymentShemeChaps : PaymentSchemeResult
{
    public override MakePaymentResult GetResult(Account account)
    {
        var result = new MakePaymentResult();
        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;
        }

        return result;
    }
}

public class PaymentSchemeBacs : PaymentSchemeResult
{
    public override MakePaymentResult GetResult(Account account)
    {
        var result = new MakePaymentResult();
        if (account == null)
        {
            result.Success = false;
        }
        else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs))
        {
            result.Success = false;
        }

        return result;
    }
}

And create a factory to create them for you. You will call the factory from your MakePayment method. Here is the factory:

public class PaymentSchemeFactory
{
    public static PaymentSchemeResult GetPaymentScheme(PaymentScheme scheme)
    {
        PaymentSchemeResult result;
        switch (scheme)
        {
            case PaymentScheme.Bacs:
                result = new PaymentSchemeBacs();
                break;
            case PaymentScheme.Chaps:
                result = new PaymentShemeChaps();
                break;
            default:
                throw new InvalidEnumArgumentException("...");
        }

        return result;
    }
}

In your MakePayment method you will do this:

if (request.PaymentScheme == PaymentScheme.FasterPayments)
{
    // Do the code for this case
}
else
{
    // Judging form your code it can be either PaymentScheme.Chaps, or 
    // PaymentScheme.Bacs. Both of them have the common interface so let's 
    // call the factory
    var paymentScheme = 
        PaymentSchemeFactory.GetPaymentScheme(request.PaymentScheme);
    result = paymentScheme .GetResult(account);
}

If you come across other code that is doing a check on PaymentScheme and then doing this or that, then you can bring that code and put it into the PaymentSchemeResult class and provide functionality for it in your derived classes. If the functionality has common parts, put that in the base class.

I do not know your business domain, but you can push more common code into these classes. I also wanted to put the account creation in the PaymentSchemeResult since both derived classes need the account. I will leave that to you.

As a side note, you can simplify your conditions from this:

if (account == null)
{
    result.Success = false;
}
else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs))
{
    result.Success = false;
}

to this:

result.Success = !(account == null) || 
    account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs);
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you, I will amend my code to reflect this and let you know how I get on. \$\endgroup\$ – user127535 Jun 18 '17 at 8:50
  • 2
    \$\begingroup\$ !(account == null) this can go wrong ;-] why not !=? \$\endgroup\$ – t3chb0t Jun 18 '17 at 9:35
1
\$\begingroup\$

Note: This answer was written after best answer was selected.

Going through a few things then putting them together.


I'd like to all PaymentSchemes handled the same even though the xxxPaymentResults signature varies.


This is common to all PaymentResults.

if (account == null)
{
    result.Success = false;
}

These parameters are common: results and account


This is also common for all three:

else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Chaps))
{
    result.Success = false;
}

BECAUSE request (which is common) has a PaymentScheme property. So we want the code to look like this if possible:

else if (!account.AllowedPaymentSchemes.HasFlag(request.Scheme))
{
    result.Success = false;
}

The xxxResults() client (the caller) creates the object that the method returns. That's not SOLID.


I'm going to borrow @CodingYoshi's idea of an abstract class.

However transform the OP's parameters so as to make a consistant abstract class so every PaymentScheme can be instantiated, and subsequently called, the same. We have all needed parameters already at the point the PaymentSchemeFactory is called, so let's use them. This helps future-proofing too.

// This structure emphasises the sameness of PaymentScheme objects.
// Clients write / override *only* what is unique for a given subclass.
// Thus subclass structure emphasizes the differences.
// Single Responsibility is therefore enhanced.

public abstract class PaymentSchemeResult
   // Do these getters need to be public?
   public Account            Account { get; protected set; }
   public MakePaymentRequest Request { get; protected set; }
   public MakePaymentResult  Result  { get; protected set; }

   public PaymentSchemeResult (Account theAccount, MakePaymentRequest theRequest) {
      // I guess we allow null arguments - see GetResults() - so no checking here

      this.Account = theAccount;    this.PaymentResult = new MakePaymentResult();
      this.Request = theRequest;
   }

  // An example of the template method pattern.
   public  MakePaymentResult GetResult()
   {
      if (Account == null) {
        Result.Success = false;
      } 
      else if (!Account.AllowedPaymentSchemes.HasFlag(PaymentResult.PaymentScheme)) {
    Result.Success = false;
      }

      GetResultOverride();
   }

   // virtural, so subclasses only override when there is actually something to do.
   protected virtual void GetResultOverride() { };
}

The Factory knows what subclass to make because the request object has that property.

public class PaymentSchemeFactory
{
   public static PaymentSchemeResult Create(Account thisAccount, MakePaymentRequest thisRequest)
   {
      PaymentSchemeResult result;

      switch (thisRequest.Scheme)
      {
         case PaymentScheme.Bacs:
            result = new PaymentSchemeBacs(thisAccount, thisScheme);
            break;
         case PaymentScheme.Chaps:
            result = new PaymentSchemeChaps(thisAccount, thisScheme);
            break;
         case PaymentScheme.FasterPayment:
            result = new PaymentSchemeFasterPayment(thisAccount, thisScheme);
            break;
         default:
            throw new InvalidEnumArgumentException(string.Format("PaymentScheme '{0}' not implemented", thisRequest.Scheme));
        }

       return result;
    }
}

Subsequently Simple Subclasses:

public class PaymentSchemeBacs : PaymentSchemeResult {
   public PaymentSchemeBacs (Account thisAccount, MakePaymentRequest thisScheme)
      : base(thisAccount, thisScheme)  { }

   // no GetResults override 
}


public class PaymentSchemeChaps : PaymentSchemeResult {
   public PaymentSchemeChaps (Account thisAccount, MakePaymentRequest thisScheme)
      : base(thisAccount, thisScheme)  { }

   public override void GetResultsOverride() {
      if (Account.Status != AccountStatus.Live)
         Result.Success = false;
   }
}

public class PaymentSchemeFasterPayment : PaymentSchemeResult {
   public PaymentSchemeFasterPayment (Account thisAccount, MakePaymentRequest thisScheme)
    : base(thisAccount, thisScheme)  {  }

   public override void GetResultsOverride() {
      if (Account.Balance < Request.Amount)
         Result.Success = false;
   }
}

Don't make class names with action verbs

  • PaymentRequest not MakePaymentRequest
  • PaymentResult not MakePaymentResult
|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

Here is my contribution:

In accordance to the open to extension, closed to modification principle, you would extract out each of the scheme payments types to a separate classes, so that if additional schemes are added you do not need to modify any existing classes, just simply create a new class that inherits from the MakePayment Request class.

 public class BacsPaymentRequest : MakePaymentRequest
  {
    public override MakePaymentResult ProcessPayment(Account account)
    {

      if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs))
      {
        return new MakePaymentResult { Success = false, Message = "Payment Failed. Bacs Payment is not supported for this account." };
      }

      return new MakePaymentResult { Success = true };
    }
  }

and

 public class ChapsPaymentRequest : MakePaymentRequest
  {
    public override MakePaymentResult ProcessPayment(Account account)
    {

      if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Chaps))
      {
        return new MakePaymentResult { Success = false, Message = "Payment Failed. Faster Payment is not supported for this account." };

      }
      if (account.Status != AccountStatus.Live)
      {
        return new MakePaymentResult { Success = false, Message = "Payment Failed. Account is Disabled." };
      }
      return new MakePaymentResult { Success = true };
    }
  }

and

public class FasterPaymentRequest : MakePaymentRequest
  {

    public override MakePaymentResult ProcessPayment(Account account)
    {

      if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.FasterPayments))
      {
        return new MakePaymentResult { Success = false, Message = "Payment Failed. Faster Payment is not supported for this account." };
      }

      if (account.Balance < Amount)
      {
        return new MakePaymentResult { Success = false, Message = "Payment Failed. Insufficient funds in account" };
      }

      return new MakePaymentResult { Success = true };
    }
  }

the payment service class can then be reduced to a much simpler class whose purpose is to only call the check account methods and then subtract any balances taken. (Perhaps the balance recalculation could be abstracted out to another class too).

    public class PaymentService : IPaymentService
    {

    private readonly IDataStore _accountDataStore;


    public PaymentService(IDataStore dataStore)
    {
      _accountDataStore = dataStore;
    }


    public MakePaymentResult MakePayment(MakePaymentRequest request)
        {

         Account account = _accountDataStore.GetAccount(request.DebtorAccountNumber);

          if (account == null)
            return new MakePaymentResult {Success = false, Message = "Payment Failed. Unable to find account details"};

            var result = request.ProcessPayment(account);

            if (result.Success)
            {
              account.Balance -= request.Amount;

              _accountDataStore.UpdateAccount(account);

            }

          return result;


        }
  }
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy