Is it bad to use ViewModelLocator to Grab other VM's for use in another Vm?

Question

I am using MVVM light and figured out since that the ViewModelLocator can be used to grab any view model and thus I can use it to grab values.

I been doing something like this

public class ViewModel1
{
   public ViewModel1()
   {
        var vm2 = new ViewModelLocator().ViewModel2;
        string name = vm2.Name;
   }
}

This way if I need to go between views I can easily get other values. I am not sure if this would be best practice though(it seems so convenient makes me wonder if it is bad practice lol) as I know there is some messenger class thing and not sue if that is the way I should be doing it.

Edit

  static ViewModelLocator()
        {
            ServiceLocator.SetLocatorProvider(() => SimpleIoc.Default);

            SimpleIoc.Default.Register<ViewModel1>();
SimpleIoc.Default.Register<ViewModel2>();
        }


  [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance",
         "CA1822:MarkMembersAsStatic",
         Justification = "This non-static member is needed for data binding purposes.")]
        public ViewModel1 ViewModel1
        {
            get
            {
                return ServiceLocator.Current.GetInstance<ViewModel1 >();
            }
        }

Edit

Here is a scenario that I am trying to solve.

I have a view that you add price and store name to. When you click on the textbox for store name you are transferred to another view. This view has a textbox that you type the store you are looking for, as you type a select list get populated with all the possible matches and information about that store.

The user then chooses the store they want. They are transferred back to the view where they "add the price", now the store name is filled in also.

If they hit "add" button it takes the price, the store name, and the barcode(this came from the view BEFORE "add price view") and sends to a server.

So as you can see I need data from different views.

Answer 1

Yes, you can do this, in as much as the code will work but there is a big potential issue you may run into in the future.

One of the strong arguments for using the MVVM pattern is that it makes it easier to write code that can be easily tested.
With you're above code you can't test ViewModel1 without also having ViewModelLocator and ViewModel2. May be that's not too much of a bad thing in and of itself but you've set a precedent that this type of strong coupling of classes is acceptable. What happens, in the future, when you

From a testing perspective you would probably benefit from being able to inject your dependencies. This means passing, to the constructor--typically, the external objects of information you need.

This could mean you have a constructor like this:

public ViewModel1(string vm2Name)
{
    string name = vm2Name;
}

that you call like this:

var vm1 = new ViewModel1(ViewModelLocator.ViewModel2.name);

There are few other issues you may want to consider also.

You're also creating a new ViewModelLocator to access one of it's properties. You probably already have an instance of the locator defined at the application level. You're creating more work for yourself (and the processor) if you're newing up additional, unnecessary instances.

Do you really need a complete instance of ViewModel2 if all you need is the name? Avoid creating and passing more than you need to.

Update

If you capture the store in the first view/vm then why not pass that (ID &/or Name) to the second VM from the second view? The second VM can then send that to the server with the data captured in the second view.

Another approach may be to just use one viewmodel for both views. This may make your whole problem go away.

Answer 2

I'm trying to understand what your scenario is. In the MVVMlight forum, you added the following context to this question:

"I have some data that needs to be passed to multiple screens and possibly back again."

For passing data between VMs, I would also - as Matt above - use the Messenger class of MVVMLight as long as it is "fire and forget". But it is the "possibly back again" comment that sounds tricky.

I can imagine some scenarios where this can be needed. Eg. a wizard interface. In such a case I would model the data that the wizard is responsible for collecting and then bind all Views to the same VM, representing that model object.

But that's just one case. So maybe if you could provide a little more context, I would be happy to try and help.

Answer 3

If you have properties in 1 view or view model that need to be accessed by a second (or additional) views or view models, I'd recommend creating a new class to store these shared properties and then injecting this class into each view model (or accessing it via the locator). See my answer here... Two views - one ViewModel

Here is some sample code still using the SimpleIoc

public ViewModelLocator() 
{  
    ServiceLocator.SetLocatorProvider(() => SimpleIoc.Default);                        
    SimpleIoc.Default.Register<IMyClass, MyClass>();              
}  

public IMyClass MyClassInstance
{
    get{ return ServiceLocator.Current.GetInstance<IMyClass>();}
} 

Here is a review of SimpleIOC - how to use MVVMLight SimpleIoc?

However, as I mentioned in my comments, I changed to use the Autofac container so that my supporting/shared classes could be injected into multiple view models. This way I did not need to instantiate the Locator to access the shared class. I believe this is a cleaner solution.

This is how I registered MyClass and ViewModels with the Autofac container-

var builder = new ContainerBuilder();
var myClass = new MyClass();
builder.RegisterInstance(myClass);
builder.RegisterType<ViewModel1>();
builder.RegisterType<ViewModel2>();
_container = builder.Build();
ServiceLocator.SetLocatorProvider(() => new AutofacServiceLocator(_container));   

Then each ViewModel (ViewModel1, ViewModel2) that require an instance of MyClass just add that as a constructor parameter as I linked initially.

MyClass will implement PropertyChanged as necessary for its properties.

Answer 4

Ok, my shot at an answer for your original question first is: Yes, I think it is bad to access one VM from another VM, at least in the way it is done in the code example of this question. For the same reasons that Matt is getting at - maintainability and testability. By "newing up" another ViewModelLocator in this way you hardcode a dependency into your view model.

So one way to avoid that is to consider Dependency Injection. This will make your dependencies explicit while keeping things testable. Another option is to use the Messenger class of MVVMLight that you also mention.

In order to write maintainable and testable code in the context of MVVM, ViewModels should be as loosely coupled as possible. This is where the Messenger of MVVMLight can help. Here's a quote from Laurent on what Messenger class was intended for:

I use it where decoupled communication must take place. Typically I use it between VM and view, and between VM and VM. Strictly speaking you can use it in multiple places, but I always recommend people to be careful with it. It is a powerful tool, but because of the very loose coupling, it is easy to lose the overview on what you are doing. Use it where it makes sense, but don't replace all your events and commands with messages.

So, to answer the more specific scenario you mention, where one view pops up another "store selection" view and the latter must set the current store when returning back to the first view, this is one way to do it (the "Messenger way"):

1) On the first view, use EventToCommand from MVVMLight on the TextBox in the first view to bind the desired event (eg. GotFocus) to a command exposed by the view model. Could be eg. named OpenStoreSelectorCommand.

2) The OpenStoreSelectorCommand uses the Messenger to send a message, requesting that the Store Selector dialog should be opened. The StoreSelectorView (the pop-up view) subscribes to this message (registers with the Messenger for that type of message) and opens the dialog.

3) When the view closes with a new store selected, it uses the Messenger once again to publish a message that the current store has changed. The main view model subscribes to this message and can take whatever action it needs when it receives the message. Eg. update a CurrentStore property, which is bound to a field on the main view.

You may argue that this is a lot of messaging back and forth, but it keeps the view models and views decoupled and does not require a lot code.

That's one approach. That may be "old style" as Matt is hinting, but it will work, and is better than creating hard dependencies.

A service-based approach: For a more service-based approach take a look at this recent MSDN Magazine article, written by the inventor of MVVMLight. It gives code examples of both approaches: The Messenger approach and a DialogService-based approach. It does not, however, go into details on how you get values back from a dialog window.

That issue is tackled, without relying on the Messenger, in this article. Note the IModalDialogService interface:

public interface IModalDialogService
{
  void ShowDialog<TViewModel>(IModalWindow view, TViewModel viewModel, Action<TViewModel> onDialogClose);

  void ShowDialog<TDialogViewModel>(IModalWindow view, TDialogViewModel viewModel);
}

The first overload has an Action delegate parameter that is attached as the event handler for the Close event of the dialog. The parameter TViewModel for the delegate is set as the DataContext of the dialog window. The end result is that the view model that caused the dialog to be shown initially, can access the view model of the (updated) dialog when the dialog closes.

I hope that helps you further!