0

Working on WPF (UI programming) in C# I've started frequently injecting methods into the constructor of my view models.

For example:

public class HtmlRegexListViewModel : ViewModel
{
   private readonly Action<HtmlRegex> onSelection;

   public List<HtmlRegex> HtmlRegexes { get; private set; }

   private HtmlRegex selected;
   public HtmlRegex Selected 
   {
      get { return selected; } 
      set 
      {
         selected = value;
         onSelection(value);  
      }
   }

   public HtmlRegexListViewModel(Action<HtmlRegex> onSelection, IDatabase database)
   {
      this.onSelection = onSelection;
      HtmlRegexes = database.LoadHtmlRegexesVerySlowly();
   }
}

Now I do this chiefly on view models because I don't want to go to the effort of creating an EventHandler and EventArgs derived pair of classes every time I want to tell the parent object that my state has changed.

However I also use this extensively elsewhere.

It seems like it's a style I'm going to regret when it comes to maintaining the code in future, but at the moment I can't think of many downsides. It decouples, preserves separation of concerns and is a nice way to make a class which has a single responsibility.

So is this a pattern (deferred double dispatch?), an anti-pattern or if it isn't, should it be?

Underscore
  • 113
  • 3
  • I imagine you create all these actions as inline anonymous functions when you constuct the vm? How do you find your debugging? – Ewan Oct 15 '16 at 17:10
  • @Ewan, the debugging is the main issue I've encountered, it tripped me up the other day when I was inspecting a collection which was cleared by a call to an anonymous function. But with the (relatively) new support for debugging these types of functions it's fairly easy. – Underscore Oct 23 '16 at 15:39

3 Answers3

1

Constructor-injecting functions isn't an anti-pattern in and of itself. And, to the contrary, if you want to be dogmatic about it, your constructor should demand everything an instance needs to be valid. (Though, usually nothing beyond what's needed for validity.)

That said, be sure to consider the prevailing dogmas for what they are: Guidelines for producing simpler, DRY-er, more maintainable code. And then think about how those rules should be manifested or ignored in achieving those end-goals in your application.


In your case, I'm not sure your constructor is the right place for that function injection. If your HtmlRegexListViewModel could be valid without the event/notification, I would personally prefer to construct it like this:

var o = new HtmlRegexListViewModel(database);
o.onSelection = () => { whatever(); };

Or this:

new HtmlRegexListViewModel(database) {
  onSelection = () => { whatever(); }
}

Or whatever.

svidgen
  • 13,414
  • 2
  • 34
  • 60
  • This is a good consideration and I hadn't really thought of it. Since it would be valid to use the object without registering for the callback/notification perhaps it should be a property/field. However I feel like that might make it harder to keep track of registrations in a debugging situation and maybe immutability makes that easier. – Underscore Oct 23 '16 at 15:44
0

I would not call it "Constructor method injection", because it is not a method, it is a delegate, and delegates are a bit too technical, and a bit too language-dependent: C# supports them, many other languages do not. We could get into an argument about the relative merits of delegates vs. single-method interfaces, but it would be moot point; instead of focusing on the technicalities of that constructor argument, I would rather focus on its purpose, which is observing events. So, I would call it "Constructor observer injection".

I have never heard of such a thing as a pattern, but I do not think it is an anti-pattern, either. You are simply creating an object which offers an event that may be delivered to one and only one observer, and mandates that this observer must be supplied at construction time.

You might slightly regret it if you ever come across the need to instantiate an object without an observer, but the damage is very small, because you can either:

  • Pass null and make your object check for null before invoking the observer, or

  • Simply pass an observer which does nothing; lambdas make this very easy.

You might regret it even a bit more if you come across the need to write a class which offers not one but 5 events, in which case passing 5 observers as constructor parameters might start to look ugly.

That having been said, why do you say that you "don't want to go to the effort of creating an EventHandler and EventArgs derived pair of classes"? Events are not limited to EventHandlers and EventArgs, you can declare an event that operates on a Action<HtmlRegex> so that you may have zero or multiple observers instead of just one observer.

Mike Nakis
  • 32,003
  • 7
  • 76
  • 111
  • You helped me realise I had a whole lot of bugs in the cases where I've used this approach without checking the action for ```null``` first, thank you for that. I also like the comparison/equivalence you draw with single method interfaces. I also didn't know about the ability to use ```Action```s instead of ```EventHandler```s which is a useful bit of knowledge to have. – Underscore Oct 23 '16 at 15:51
  • Oh yes, events with any kind of delegate instead of ol' `EventHandler` are quite handy. – Mike Nakis Oct 23 '16 at 16:06
  • However, keep in mind that single-method interfaces are somewhat frowned upon in C#, and delegates are preferred instead. C# gives you lots of candy when you use delegates, such as the ability to use lambdas, while it does not do that with single-method interfaces. On the other hand, other languages like java do not have delegates, but they allow you to use lambdas in place of single-method interfaces. With each language, it is best to use the features that are specific to it. It is just useful to know, when coding in C#, that your delegates are in actuality single-method interfaces. – Mike Nakis Oct 23 '16 at 16:07
0

In an OOP interpretation, the Action<HtmlRegex> onSelection is an example of the Strategy Pattern. You've parametrized your class over an onSelection operation which is dispatched dynamically – any supplied action will work as long as it conforms to the Action<HtmlRegex> interface. While the Action is a built-in type, you could have also defined your own interface to the same effect, though this would have been more cumbersome for users:

interface SelectionHandler
{
  void Handle(HtmlRegex r);
}

...

class HtmlRegexListViewModel
{
  ...
  public HtmlRegexListViewModel(SelectionHandler onSelection, IDatabase database)
  {
    ...
  }
}

Injecting a strategy dependency through the constructor is the usual way to do it, but any dependency injection technique is possible.

In a functional programming interpretation, this is just a callback. There's nothing special here, and it would be far too common to have a particular name.

amon
  • 132,749
  • 27
  • 279
  • 375
  • The Strategy Pattern seems to be the best description/name for it. I think this helps clarify the point for me that the use of actions is effectively a shortcut/lazy implementation of single method interfaces (in this scenario). – Underscore Oct 23 '16 at 15:49