Was going through some legacy code recently and found a big for loop that had multiple switch statement inside. So being that I hate something that is fairly large, every method should be small and concise, I went along to refactor it to SRP.
This code is generic so you might see something like this in your code base also.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 |
IEnumerable<Customer> allCustomers = Respository.GetAllCustomers(); IDictionary<string, string> payingCustomers = new Dictionary<string, string>(); IDictionary<string, string> nonPayingCustomers = new Dictionary<string, string>(); IDictionary<string, string> deadBeatCustomers = new Dictionary<string, string>(); foreach(var cust in allCustomers) { if(cust.Type == IGNORE) continue; switch(type) { case A_TYPE: payingCustomer.Add(cust.Key, cust.Name); break; case B_TYPE: nonPayingCustomer.Add(cust.Key, cust.Name); break; case C_TYPE: deadBeatCustomer.Add(cust.Key, cust.Name); break; } } |
As you will notice one can go on and keep adding types to it (so did the legacy code).
But if you think about it what this code is basically doing is that its just filtering things out to seperate collections.
Let see how we can answer things with LINQ on this one
First lets remove the if statement for customer IGNORE.
|
1 2 3 4 5 6 7 8 9 10 11 12 |
foreach(var cust in allCustomers.Where(x => x.Type != IGNORE) { switch(type) { case A_TYPE: payingCustomer.Add(cust.Key, cust.Name); break; case B_TYPE: nonPayingCustomer.Add(cust.Key, cust.Name); break; case C_TYPE: deadBeatCustomer.Add(cust.Key, cust.Name); break; } } |
Ok one step done thats better, now lets see that the loop is basically a filter, so lets just take the payingCustomer example and break it down first. Its is going through a loop and finding the A_TYPE customer. Therefore a linq query like
collection.Where(x => x.Type == A_TYPE).toDictionary(t => t.Key, t.Name);
can answer that one.
So lets create a function/method that does this.
|
1 2 3 4 5 |
private static IDictionary<string, string> FilterCustomers(IEnumerable<Customer> customers, Type type) { if(customers == null) throw ArgumentNullException(); return customers.Where(x => x.Type == type).toDictionary(t => t.Key, t.Name); } |
So here we have a single responsibility function, that just does one thing and does it well.
Its more of a functional style of programming (data in and data out).
No side effects of any sort. No saving of data to database or file etc.
Now lets go back to our original code, we can now remove the loop and switch statement all together.
|
1 2 3 4 5 6 7 8 9 |
IEnumerable<Customer> allCustomers = Respository.GetAllCustomers(); var filterCustomer = allCustomers.Where(x => x.Type != IGNORE); var payingCustomers = FilterCustomers(filterCustomer, A_TYPE); var nonPayingCustomers = FilterCustomers(filterCustomer, B_TYPE); var deadBeatCustomers = FilterCustomers(filterCustomer, C_TYPE); |
Note: we now have a filterCustomer collection, which filters out the ignore customers first.
By doing so we can write a unit test to test the method/function FilterCustomers and we can be 100% sure that all it does is a single responsibility task.
We now have removed complexity in our codebase and the test-ability goes up.
Leave A Comment