I came across batch job code similar to the below ( the below is just for an example and far simpler than reality!) that was written to update orders in a database from an xml response from a payment system. The code should do the following:
- Load the response into an object we can read the data from.
- Get a list of ‘batches’ returned in the data.
- Update the batches as received in our database.
- Get a list of orders for each batch and update them accordingly in our database.
The original code does what it’s supposed to do but I wished to put unit tests around it. What I found was it was difficult to do because the entry point for my mocks was at the highest level. The class was also doing three things, getting and parsing the payment response, updating a batch in our system and updating orders in our system. If I wasn’t careful tests I wrote to tests that orders are updated correctly could break because of changes to how batches are updated, not good. I figured I should do some refactoring so that functionality was better broken out into classes that only had one responsibility.
The highest level method itself is a transaction processing center. It should have many things to do, but it should not do them directly. For instance, the parsing of the result is delegated already to a class that has one job, parse response data so it is accessible to the system. This class is the ‘PaymentResponseReader’. It seems like the usage of this class is ok. What I think needs better abstraction is the processing of the batches and orders. I don’t think the transaction center need know about how batches of orders and orders are updated as this behavior could change and these changes should not force a change on the batch job class itself. To this end I created a ‘OrderBatchResponseUpdater’ class and a ‘OrderResponseUpdater’ class. I push down the logic for handling order batch updating and order updating into these classes. In this manner if there is a change in how orders are updated, or how batch orders are updated, it will only affect their respective classes. Also it will make this functionality much easier to test.
Below is what I came up with:
‘ProcessOrders’ no longer is coupled to the data access layer. I’ve created objects that each have a single responsibility, or a single reason to change, and those are bound to the data layer. The ‘ProcessOrers’ method is also now much simpler to test as I can mock my new classes. Below are the new classes. They are also much easier to test as their behavior is isolated. I only have to mock the data access layer, not a file service that has nothing to do with their behavior.
I don’t know that my solution is optimal but I think it is better than what was there originally. I feel better about the cohesion and coupling of the system and my testing has become simpler, so I’m happy!