Thursday, November 29, 2012

A bad way to set up commands in your view model

tl;dr; Put code where it is defined. It makes maintenance much easier.

This has become a bit of a pet-peeve of mine lately. Please indulge me while I share.

I spend a lot of time working with code that was started by someone else.
As MVVM Light is used in most projects I work on, I see code that looks like this a lot:

    public class MyViewModel : ViewModelBase
    {
        public MyViewModel()
        {
            MyCommand = new RelayCommand(() =>
            {
               // some functionality here
            });
        }
        
        public RelayCommand MyCommand { get; private set; }
    }
This bugs me.
Seeing this just makes me think that the original developer was planning on allowing for the possibility that they may want to change what the command does at some point in the future.
But I haven't seen any code that actually does that, yet.

Remember Y.A.G.N.I.
I think this is much better:
    public class MyViewModel : ViewModelBase
    {
        public MViewyModel()
        {
        }
        
        public RelayCommand MyCommand 
        { 
            get
            {
                return new RelayCommand(() =>
                {
                    // some functionality here
                });
            }
        }
    }
 
And you probably don't really need the empty constructor. (I know in this instance you don't but depending on what else is being done in the VM you might.)
There is no difference in the actual number of lines of code or functionality but it's much better.

Why do I think this is better?

It puts the actual code where it is defined and so when I identify the command I'm after I can see what it does.
Putting the code where it is defined means it's easier to navigate the code when you aren't familiar with it.

Let's say I need to change (or fix) some behaviour that is triggered via a command. I'll start in the view (the UI) which uses/triggers that command. From there I can easily see where the command is defined (Ctrl + Click : with ReSharper). That's good, but if the code isn't there I have to search the code ("Find Usages" or "Find References") and then go to where I think it's most likely to be based on what I can see in the search results.
I've had to, unnecessarily, go hunting around the code for what I want.
The code I'm looking for has been hidden!
That's not a good way to treat the people who will be maintaining the code you wrote.

Not wanting to make any claims about my mental health and disposition but:
"Always code as if the person who ends up maintaining your code is a violent psychopath who knows where you live."


Am I missing something by doing this my way?
Does the maintainability benefit come at some other cost?

Obviously there are exceptions to the above and times when you may not want to have a readonly command. My point is to think about the code you're writing and the people who will be maintaining it and trying to work out what it does. The easier you can make this for them the less frustrated with you they'll get. :)



9 comments:

  1. You are returning a new instance of MyCommand every time the get property is requested this way.

    e.g
    if(this.MyCommand == this.MyCommand)
    { Console.WriteLine("same");
    }

    This would never be true as both requests to MyCommand will return two separate instances.

    you would need a private backing field to keep the first reference of MyCommand and return that for subsequent requests

    private RelayCommand _myCommand;
    public RelayCommand MyCommand
    {
    get
    {
    if(_myCommand == null)
    {
    _myCommand = new RelayCommand(() =>
    {
    // some functionality here
    });
    }
    return _myCommand;
    }
    }

    Sorry, formatting probably is not pretty.

    ReplyDelete
  2. Anonymous5:13 pm

    I think it's a bad habit to put a method's code inline into a property getter. It's harder to read and beguiles into writing duplicate code. We use conventions for that, e.g. if the command is named "SaveCommand" the relay would point to a method named "Save". If such a convention is followed by anyone in the team navigation is easy PLUS you have a nice separation of properties and methods, as it should be IMO.

    @Mister_Goodcat

    ReplyDelete
  3. Anonymous9:05 pm

    Hover your cursor over the class and press f12. Navigation solved.

    ReplyDelete
  4. @anonymous how to do the navigation isn't an issue, it's that I've had to do it over and over and over again recently.

    ReplyDelete
  5. Anonymous12:33 am

    The method-on-class-then-put-it-in-a-command pattern also adds the nice functionality that if you do end up having to call it in code, you don't have to do

    myinstance.mycommand.execute(param);

    While not windows-phone-y, the vb version of that is even worse, so getting rid of that is even better:

    myinstance.mycommand.execute.invoke(new object(){param})

    ReplyDelete
  6. I don't agree with either approach. Why not declare your command's behavior in a separate class, code to an interface ICommand.. then use dependency injection to determine the command that is executed at runtime. This makes your code much easier to control in testing scenarios or any other scenario where you would want to switch out the command without changing code. That inline delegate stuff is the definition of locking you into a concrete implementation..

    Check out my Blog

    ReplyDelete
  7. @BuddyJames But are you really writing tests for your commands in a phone app that are providing real value? and how many times have you needed to change an implementation?
    While that all sounds nice and [allegedly] "best practice" in principle, in my reality it just seems like overkill.

    ReplyDelete
  8. I don't think that there is any dispute about value in unit testing, Dependency Injection, loose coupling in your design, or even programming to an interface and not a concrete implementation. I do write unit tests for each layer in my application. My layers are appropriately decoupled because I use dependency injection and I write interfaces for my dependencies. Check out this article I wrote on Dependency injection with Microsoft unity on my blog.

    I hope this helps.

    ReplyDelete
  9. This comment has been removed by the author.

    ReplyDelete

I get a lot of comment spam :( - moderation may take a while.