Sunday, September 9, 2012

To constructor or to property dependency?

To constructor or to property dependency?:
Us, devel­op­ers, are a bit like that comic strip (from always great xkcd):
Crazy Straws
We can end­lessly debate over tabs ver­sus spaces (don't even get me started), whether to use optional semi­colon or not, and other seem­ingly irrel­e­vant top­ics. We can have heated, informed debates with a lot of merit, or (much more often) not very con­struc­tive exchanges of opinions.
I men­tion that to explic­itly point out, while this post might be per­ceived as one of such dis­cus­sions, the goal of it is not to be a flame­war bait, but to share some experience.
So hav­ing said that, let's cut to the chase.

Depen­den­cies, mechan­ics and semantics

Writ­ing soft­ware in an object ori­ented lan­guage, like C#, fol­low­ing estab­lished prac­tices (SOLID etc) we tend to end up with many types, con­cen­trated on doing one thing, and del­e­gat­ing the rest to others.
Let's take an arti­fi­cial exam­ple of a class that is sup­posed to han­dle sce­nario where a user moved and we need to update their address.
public class UserService: IUserService
{
   // some other members

   public void UserMoved(UserMovedCommand command)
   {
      var user = session.Get<User>(command.UserId);

      logger.Info("Updating address for user {0} from {1} to {2}", user.Id, user.Address, command.Address);

      user.UpdateAddress(command.Address);

      bus.Publish(new UserAddressUpdated(user.Id, user.Address));
   }
}
There are four lines of code in this method, and three dif­fer­ent depen­den­cies are in use: ses­sion, log­ger and bus. As an author of this code, you have a few options of sup­ply­ing those depen­den­cies, two of which that we're going to con­cen­trate on (and by far the most pop­u­lar) are con­struc­tor depen­den­cies and prop­erty dependencies.

Tra­di­tional school of thought

Tra­di­tional approach to that prob­lem among C# devel­op­ers goes some­thing like this:
Use con­struc­tor for required depen­den­cies and prop­er­ties for optional dependencies.
This option is by far the most pop­u­lar and I used to fol­low it myself for a long time. Recently how­ever, at first uncon­sciously, I moved away from it.
While in the­ory it's a neat, arbi­trary rule that's easy to fol­low, in prac­tice I found it is based on a flawed premise. The premise is this:
By look­ing at the class' con­struc­tor and prop­er­ties you will be able to eas­ily see the min­i­mal set of required depen­den­cies (those that go into the con­struc­tor) and optional set that can be sup­plied, but the class doesn't require them (those are exposed as properties).
Fol­low­ing the rule we might build our class like that:
public class UserService: IUserService
{
   // some other members

   public UserService(ISession session, IBus bus)
   {
      //the obvious
   }

   public ILogger Logger {get; set;}
}
This assumes that ses­sion and bus are required and log­ger is not (usu­ally it would be ini­tialised to some sort of NullLogger).
In prac­tice I noticed a few things that make use­ful­ness of this rule questionable:
  • It ignores con­struc­tor over­loads. If I have another con­struc­tor that takes just ses­sion does it mean bus is an optional or manda­tory depen­dency? Even with­out over­load­ing, in C# 4 or later, I can default my con­struc­tor para­me­ters to null. Does it make them required or mandatory?
  • It ignores the fact that in real­ity I very rarely, if ever, have really optional depen­den­cies. Notice the first code sam­ple assumes all of its depen­den­cies, includ­ing log­ger, are not null. If it was truly optional, I should prob­a­bly pro­tect myself from Null­Ref­er­ence­Ex­cep­tions, and in the process com­pletely destroy read­abil­ity of the method, allow­ing it to grow in size for no real benefit.
  • It ignores the fact that I will almost never con­struct instances of those classes myself, del­e­gat­ing this task to my IoC con­tainer. Most mature IoC con­tain­ers are able to han­dle con­struc­tor over­loads and defaulted con­struc­tor para­me­ters, as well as mak­ing prop­er­ties required, ren­der­ing the argu­ment about required ver­sus optional moot.
Another, more prac­ti­cal rule, is this:
Use con­struc­tor for not-changing depen­den­cies and prop­er­ties for ones that can change dur­ing object's lifetime.
In other words, ses­sion and bus would end up being pri­vate read­only fields — the C# com­piler would enforce that once we set them (option­ally val­i­dat­ing them first) the fields in the con­struc­tor, we are guar­an­teed to be deal­ing with the same (cor­rect, not null) objects ever after. On the other hand, Log­ger is up in the air, since tech­ni­cally at any point in time some­one can swap it for a dif­fer­ent instance, or set it to null. There­fore, what usu­ally log­i­cally fol­lows from there, is that prop­erty depen­den­cies should be avoided and every­thing should go through the constructor.
I used to be the fol­lower of this rule until quite recently, but then it does have its flaws as well.
  • It leads to some nasty code in sub­classes where the base class has depen­den­cies. One exam­ple I saw recently was a WPF view model base class with depen­dency on dis­patcher. Because of that every sin­gle (and there were many) view model inher­it­ing from it, needed to have a con­struc­tor declared that takes dis­patcher and passes it up to the base con­struc­tor. Now imag­ine what hap­pens when you find you need event aggre­ga­tor in every view model. You will need to alter every sin­gle view model class you have to add that, and that's a refac­tor­ing ReSharper will not aid you with.
  • It trusts the com­piler more than devel­op­ers. Just because a set­ter is pub­lic, doesn't mean the devel­op­ers will write code set­ting it to some ran­dom val­ues all over the place. It is all a mat­ter of con­ven­tions used in your par­tic­u­lar team. On the team I'm cur­rently work­ing with the rule that every­body knows and fol­lows is we do not use prop­er­ties, even set­table ones, to reas­sign depen­den­cies, we're using meth­ods for that. There­fore, based on the assump­tion that devel­op­ers can be trusted, when read­ing the code and see­ing a prop­erty I know it won't be used to change state, so read­abil­ity and imme­di­ate under­stand­ing of the code does not suffer.
In real­ity, I don't actu­ally think the cur­rent one, or few of my last projects had even a require­ment to swap ser­vice depen­den­cies. Gen­er­ally you will direct your depen­dency graphs from more to less volatile objects (that is object will depend on objects with equal or longer lifes­pan). In few cases where that's not the case the depen­den­cies would be pulled from a fac­tory, and used only within the scope of a sin­gle method, there­fore not being avail­able to the out­side world. The only sce­nario where a long-lived depen­dency would be swapped that I can think of, is some sort of failover or circuit-breaker, but then again, that would be likely dealt with inter­nally, inside the component.
So, look­ing back at the code I tend to write, all depen­den­cies tend to be manda­tory, and all of them tend to not change after the object has been created.

What then?

This robs the afore­men­tioned approaches from their adver­tised ben­e­fits. As such I tend to draw the divi­sion along dif­fer­ent lines. It does work for me quite well so far, and in my mind, offers a few ben­e­fits. The rule I fol­low these days can be explained as follows:
Use con­struc­tor for application-level depen­den­cies and prop­er­ties for infra­struc­ture or inte­gra­tion dependencies.
In other words, the depen­den­cies that are part of the essence of what the type is doing go into the con­struc­tor, and depen­den­cies that are part of the "back­ground" go via properties.
Using our exam­ple from before, with this approach the ses­sion would come via con­struc­tor. It is part of the essence of what the class is doing, being used to obtain the very user whose address infor­ma­tion we want to update. Log­ging and pub­lish­ing the infor­ma­tion onto the bus are some­what less essen­tial to the task, being more of an infra­struc­ture con­cerns, there­fore bus and log­ger would be exposed as properties.
  • This approach clearly puts dis­tinc­tion between what's essen­tial, busi­ness logic, and what is book­keep­ing. Prop­er­ties and fields have dif­fer­ent nam­ing con­ven­tions, and (espe­cially with ReSharper's 'Color iden­ti­fiers' option) with my code colour­ing scheme have sig­nif­i­cantly dif­fer­ent colours. This makes is much eas­ier to find what really is impor­tant in code.
  • Given the infra­struc­ture level depen­den­cies tend to be pushed up to base classes (like in the View­Model exam­ple with Dis­patcher and Even­tAg­gre­ga­tor) the inher­i­tors end up being leaner and more readable.

In clos­ing

So there you have it. It may be less pure, but trades purity for read­abil­ity and reduces amount of boil­er­plate code, and that's a trade­off I am happy to make.


DIGITAL JUICE

No comments:

Post a Comment

Thank's!