Refactoring complex classes – when more IoC doesn’t quite cut it

Share the joy
  •  
  •  
  •  
  •  

One of the standard things we look for in potential candidates is an appreciation of the SOLID design principles.  Any developer worth his salt will probably know them, and can apply most of them  when they need to, but in everyday life, they sometimes fall by the wayside.  This is especially true when inheriting a complex code base, which has grown organically over the years; it can be a question of get in, make your changes (with tests!), and get out again as quickly as you can.  This approach is understandable of course, and it does not mean we are wild-west coders; we will still use Dependency Injection, we still cover our code with a suite of tests, but over time, we can forget to pay down our technical debt.  For this post, I’d like to tell a short story of a targeted factoring we performed, paying close attention to SOLID principles, and how it made our life so much better.

The technical debt collector calls

Although JustGiving.com may look like a fairly bog-standard, medium sized website from a technical perspective, there is an incredible amount of computation going on in the background that you probably aren’t aware of. Aside from the (six) hundreds of git repos, some of our processes are also complex, and I happen to work intimately with one; that payment processing agent.  Due to our scale, we do a lot of card-processing in house, with all of the risk that entails.  Suffice to say that after you ‘complete’ a donation, that is not the end of the story by a long shot; to us, you have just started a process by giving us enough details to start working on your payment.

Many years ago, when our payment agent was first built, our processes were a lot less sophisticated than they are now.  Upon receiving an incoming card-payment message, it would essentially choose a payment gateway, make a payment and store a result.  This work could be very easily orchestrated by a single class, with injected dependencies.  Testing was simple enough, and all was good in the world.  Over time however, certain manual processes were folded into the payment agent, one at a time over the course of a year.

Due to operational and security reasons, I can’t say exactly what we do when processing a payment, but at a high level, it started to look something like this,  with each step being able to veto a payment, returning a notification object from the orchestration method:

  • Choose a payment provider
  • Check card validity
  • Do fraud stuff
  • Submit the payment to the payment gateway
  • Some payment result handling
  • More fraud stuff
  • Optionally perform some payment rescheduling
  • Build a notification for certain system notifiers

Adding each step exacerbated two main problems: the orchestrator class got harder to understand, with multiple branches, and each new dependency broke unit tests.  After injecting a mock into each broken test, the developer had to understand how the mock should be configured in each given scenario to set up the correct state for the test. Our orchestrator class was clearly breaking two SOLID principles – it was not single responsibility any more – orchestration was becoming complex, and it violated the Open-Closed principle – each change required the orchestration class to be opened up for modification.

The straw that finally broke the camel’s back was a story to make the payment agent idempotent.  Until now, any exceptions in the process placed the payment into a re runnable state, but they had to be manually checked to ensure double-payments were not created. As we tried to automate payment retries, we had to ensure that we only ran the correct stages of the process. Specifically, if we had captured the payment (the step in bold), we should only run subsequent steps during a retry.  This was possible, but the poor orchestrator was horrendously complex, and testing became burdensome.

Paying it down

The solution to our problem was to create a payment pipeline.  Each step was  isolated, their responsibilities were very tightly defined, and their dependencies slim.  There were two types of step (one for pre-payment, and one for post), but apart from that, it was easy to split up the code into easily-testable chunks, with the following two interfaces:

Now, it is super-simple to extract the code into classes

Each step in the process need not know about any other; it is down to the pipeline creation to assemble it.  We happen to use Ninject in this application, so the assembly looks something akin to this:

Our orchestrator is now fit for human consumption.  We inject the two pre-configured pipelines into it, and it does not care what they do – it is also now single responsibility.

In addition to being simple-responsibility, out code now adheres to the open/closed principle – the pipeline can be extended by adding new steps, whilst keeping the original steps entirely intact.

Our tests become very simple; in most cases, we have two (mspec) tests per class – one happy path test and one where a rule is tripped.  It is relatively simple to set up the rules to make a stage trip, and easy to assert that the next stage is called or not.

Finally, we need to assert that the pipeline is hooked up correctly, which can be achieved via an integration test that checks the pipeline’s object graph:

Happy Days

Now that we have split out payment orchestrator into a pipeline, rationalising the process is much simpler, testing is simpler, and the whole process is much less daunting for new developers. In the future, we may add more exotic types of payment processing, which may share some aspects of the current process, whilst having their own unique process.  This is no problem for us, since we can simply create new pipelines out of our predefined components as we see fit.

Pipelines aren’t for every occasion; and stack traces can be a little verbose when something goes wrong, but they are a fairly elegant solution, that lets the developer reason about complex tasks in easy-to-digest chunks.  I hope this inspires you to consider pipelines in your travels.

 

Image courtesty of Paolo Piscolla –
https://www.flickr.com/photos/20912428@N04/2699572160


Share the joy
  •  
  •  
  •  
  •  

About the author

Jon Bates

Tinkerer; Senior developer in the FinTech team

View all posts

Leave a Reply

Your email address will not be published. Required fields are marked *