public class OrderProvider
{
public ICollection<Order> GetOrdersFor(Customer someCustomer)
{
ICollection<Order> orders = CallSomethingHereToFetchOrders();
return orders;
}
}
ICollection<Order> orderlist = OrderProvider.GetOrdersFor(someCustomer);
if(orderlist == null || orderlist.Count == 0)
{
//something is happening here//
}
else
{
//something else is happening here//
}
At a glance, someone I showed this to said there was nothing inherently wrong to this snippet. I care to disagree. What’s wrong here, in my opinion, is that the provider doesn’t have a unambiguous behavior as far as an empty list is concerned. There should either return an empty collection by default, or throw an exception if it somehow gets a null value to return. A simple implementation would be as follows.
public class OrderProvider
{
public ICollection<Order> GetOrdersFor(Customer someCustomer)
{
ICollection<Order> orders = CallSomethingHereToFetchOrders();
return orders ?? new List<order>();
}
}
ICollection<Order> orderlist = OrderProvider.GetOrdersFor(someCustomer);
if(orderlist.Count == 0)
{
//something is happening here//
}
else
{
//something else is happening here//
}
Notice we split up the complexity of the if statement in the calling code, reducing the amount of possible conditions that match the case. We also moved the responsibility of checking the validity of the model to where it belongs: the code actually BUILDING the model we’re going to use. By doing small refactoring operations like these, it’s easy to maintain a good separation of concerns. The calling code, whatever it will do (maybe… print the orders one by one to paper, or show some nice statistics on screen), will never have to worry about null checks on things like this. Now we can focus on the happy part of that bit of code!
Geen opmerkingen:
Een reactie posten