Search code examples
c#mvvm-lighticommand

Is it safe to call new RelayCommand (ICommand) in Expression-Bodied Properties


With expression bodied properties we can create a RelayCommand as follows

public RelayCommand Command => _command ?? (_command = new RelayCommand(CommandExecute));

However this is possible also

public RelayCommand Command => new RelayCommand(CommandExecute);

Obviously this creates a new RelayCommand every time the Property getter is called. Though there is comments i have seen around that says the underlying plumbing only creates one command...

Does anyone have a definitive answer on this?


Solution

  • Does anyone have a definitive answer on this?

    The documentation does not promise to only retrieve the property value once. So, you must assume it could retrieve it more than once.

    Of course, in practice this assumed behavior may never occur. It would make perfect sense that a property, once retrieved, would never be retrieved again if property-changed notification never happened for that property, and of course a read-only property will never have a property-changed notification.

    So you can probably get away with it. But personally, I wouldn't risk it. If the underlying implementation changes, or your assumption is wrong for any reason, having two or more instances of the same command is going to be a problem, at least if that command ever raises the CanExecuteChanged event. I mean, I guess if the CanExecute() state never changes, you can have as many copies of the object as you want, and they'll all work exactly the same. But if it can change, then you could wind up raising the event on the wrong command object, one that no one is listening to.

    This is not just academic. Not only could Microsoft, or some other implementer of a XAML/MVVM-based API your code might one day be used with, choose to forego storing the command object reference and instead count on being able to always retrieve it from the model object, it is common practice within the model object itself to retrieve the command object from the property. Scenarios where the command property is read multiple times are entirely plausible and worth worrying about.

    More to the point, I don't see the motivation behind either option. The "create a new one each time" seems clearly wrong to me, even if you can get away with it. And lazy initialization seems like over-complicated code for no benefit. After all, immediately after the model object is created, the next thing that almost always happens is that the properties are bound to UI, and so the command property is going to be retrieved at that time. Lazy initialization will delay initialization of the underlying field by milliseconds at most (and usually much less time than that).

    And if you forego lazy init, you get to use automatic properties:

    public RelayCommand Command { get; } = new RelayCommand(CommandExecute);
    

    No explicit field! Much nicer, IMHO.

    Noting, of course, that to use that syntax CommandExecute() would have to be a static member. Most commands do need access to the model instance and so the above wouldn't work for those.

    It's possible one reason the lazy-init pattern got popular is that it allows use of field initializer syntax, as a loophole over the usual "not allowed to use instance members in field initializers" rule.

    Personally, I'd still opt for in-constructor initialization (which works fine for read-only automatic properties…you still don't need the explicit backing field) for commands that use the current instance. Lazy-init seems like a premature and false optimization in this scenario.