Caliburn.Micro nested ViewModels best practice

Question

This is a pretty long question, so please bear with me.

Currently I am developing a small tool intended to help me keep track of the myriad of characters in my Stories.

The tool does the following:

• Load the characters which are currently stored as json on the disk and stores them in a list, which is presented in the Shell via a ListBox.
• If the user then opens a character the Shell, which is a Conductor<Screen>.Collection.OneActive, opens a new CharacterViewModel, that derives from Screen.
• The Character gets the Character that is going to be opened via the IEventAggregator message system.
• The CharacterViewModel furthermore has various properties which are sub ViewModels which bind to various sub Views.

And here is my Problem: Currently I initialize the sub ViewModels manually when the ChracterViewModel is initialized. But this sounds fishy to me and I am pretty sure there is a better way to do this, but I cannot see how I should do it.

Here is the code of the CharacterViewModel:

/// <summary>ViewModel for the character view.</summary>
public class CharacterViewModel : Screen, IHandle<DataMessage<ICharacterTagsService>>
{
    // --------------------------------------------------------------------------------------------------------------------
    // Fields
    // -------------------------------------------------------------------------------------------------------------------

    /// <summary>The event aggregator.</summary>
    private readonly IEventAggregator eventAggregator;

    /// <summary>The character tags service.</summary>
    private ICharacterTagsService characterTagsService;

    // --------------------------------------------------------------------------------------------------------------------
    // Constructors & Destructors
    // -------------------------------------------------------------------------------------------------------------------

    /// <summary>Initializes a new instance of the <see cref="CharacterViewModel"/> class.</summary>
    public CharacterViewModel()
    {
        if (Execute.InDesignMode)
        {
            this.CharacterGeneralViewModel = new CharacterGeneralViewModel();

            this.CharacterMetadataViewModel = new CharacterMetadataViewModel();
        }
    }

    /// <summary>Initializes a new instance of the <see cref="CharacterViewModel"/> class.</summary>
    /// <param name="eventAggregator">The event aggregator.</param>
    [ImportingConstructor]
    public CharacterViewModel(IEventAggregator eventAggregator)
        : this()
    {
        this.eventAggregator = eventAggregator;
        this.eventAggregator.Subscribe(this);
    }

    // --------------------------------------------------------------------------------------------------------------------
    // Properties
    // -------------------------------------------------------------------------------------------------------------------

    /// <summary>Gets or sets the character.</summary>
    public Character Character { get; set; }

    /// <summary>Gets or sets the character general view model.</summary>
    public CharacterGeneralViewModel CharacterGeneralViewModel { get; set; }

    /// <summary>Gets or sets the character metadata view model.</summary>
    public CharacterMetadataViewModel CharacterMetadataViewModel { get; set; }

    /// <summary>Gets or sets the character characteristics view model.</summary>
    public CharacterApperanceViewModel CharacterCharacteristicsViewModel { get; set; }

    /// <summary>Gets or sets the character family view model.</summary>
    public CharacterFamilyViewModel CharacterFamilyViewModel { get; set; }

    // --------------------------------------------------------------------------------------------------------------------
    // Methods
    // -------------------------------------------------------------------------------------------------------------------

    /// <summary>Saves a character to the file system as a json file.</summary>
    public void SaveCharacter()
    {
        ICharacterSaveService saveService = new JsonCharacterSaveService(Constants.CharacterSavePathMyDocuments);

        saveService.SaveCharacter(this.Character);

        this.characterTagsService.AddTags(this.Character.Metadata.Tags);
        this.characterTagsService.SaveTags();
    }

    /// <summary>Called when initializing.</summary>
    protected override void OnInitialize()
    {
        this.CharacterGeneralViewModel = new CharacterGeneralViewModel(this.eventAggregator);
        this.CharacterMetadataViewModel = new CharacterMetadataViewModel(this.eventAggregator, this.Character);
        this.CharacterCharacteristicsViewModel = new CharacterApperanceViewModel(this.eventAggregator, this.Character);
        this.CharacterFamilyViewModel = new CharacterFamilyViewModel(this.eventAggregator);

        this.eventAggregator.PublishOnUIThread(new CharacterMessage
        {
            Data = this.Character
        });


        base.OnInitialize();
    }

    /// <summary>
    /// Handles the message.
    /// </summary>
    /// <param name="message">The message.</param>
    public void Handle(DataMessage<ICharacterTagsService> message)
    {
        this.characterTagsService = message.Data;
    }
}

For Completion Sake I also give you one of the sub ViewModels. The others a of no importance because they are structured the same way, just perform different tasks.

/// <summary>The character metadata view model.</summary>
public class CharacterMetadataViewModel : Screen
{
    /// <summary>The event aggregator.</summary>
    private readonly IEventAggregator eventAggregator;

    /// <summary>Initializes a new instance of the <see cref="CharacterMetadataViewModel"/> class.</summary>
    public CharacterMetadataViewModel()
    {
        if (Execute.InDesignMode)
        {
            this.Character = DesignData.LoadSampleCharacter();
        }
    }

    /// <summary>Initializes a new instance of the <see cref="CharacterMetadataViewModel"/> class.</summary>
    /// <param name="eventAggregator">The event aggregator.</param>
    /// <param name="character">The character.</param>
    public CharacterMetadataViewModel(IEventAggregator eventAggregator, Character character)
    {
        this.Character = character;

        this.eventAggregator = eventAggregator;
        this.eventAggregator.Subscribe(this);
    }

    /// <summary>Gets or sets the character.</summary>
    public Character Character { get; set; }

    /// <summary>
    /// Gets or sets the characters tags.
    /// </summary>
    public string Tags
    {
        get
        {
            return string.Join("; ", this.Character.Metadata.Tags);
        }

        set
        {
            char[] delimiters = { ',', ';', ' ' };

            List<string> tags = value.Split(delimiters, StringSplitOptions.RemoveEmptyEntries).ToList();

            this.Character.Metadata.Tags = tags;
            this.NotifyOfPropertyChange(() => this.Tags);
        }
    }
}

I already read in on Screens, Conductors and Composition, IResult and Coroutines and skimmed the rest of the Documentation, but somehow I cannot find what I am looking for.

//edit: I should mention the code I have works just fine. I'm just not satisfied with it, since I think I am not understanding the concept of MVVM quite right and therefore make faulty code.

Answer 1

There is nothing wrong with having one ViewModel instantiate several child ViewModels. If you're building a larger or more complex application, it's pretty much unavoidable if you want to keep your code readable and maintainable.

In your example, you are instantiating all four child ViewModels whenever you create an instance of CharacterViewModel. Each of the child ViewModels takes IEventAggregator as a dependency. I would suggest that you treat those four child ViewModels as dependencies of the primary CharacterViewModel and import them through the constructor:

[ImportingConstructor]
public CharacterViewModel(IEventAggregator eventAggregator,
                            CharacterGeneralViewModel generalViewModel,
                            CharacterMetadataViewModel metadataViewModel,
                            CharacterAppearanceViewModel appearanceViewModel,
                            CharacterFamilyViewModel familyViewModel)
{
    this.eventAggregator = eventAggregator;
    this.CharacterGeneralViewModel generalViewModel;
    this.CharacterMetadataViewModel = metadataViewModel;
    this.CharacterCharacteristicsViewModel = apperanceViewModel;
    this.CharacterFamilyViewModel = familyViewModel;

    this.eventAggregator.Subscribe(this);
}

You can thus make the setters on the child ViewModel properties private.

Change your child ViewModels to import IEventAggregator through constructor injection:

[ImportingConstructor]
public CharacterGeneralViewModel(IEventAggregator eventAggregator)
{
    this.eventAggregator = eventAggregator;
}

In your example, two of those child ViewModels are passed an instance of the Character data in their constructors, implying a dependency. In these cases, I would give each child ViewModel a public Initialize() method where you set the Character data and activate the event aggregator subscription there:

public Initialize(Character character)
{
    this.Character = character;
    this.eventAggregator.Subscribe(this);   
}

Then call this method in your CharacterViewModel OnInitialize() method:

protected override void OnInitialize()
{    
    this.CharacterMetadataViewModel.Initialize(this.Character);
    this.CharacterCharacteristicsViewModel.Initialize(this.Character);    

    this.eventAggregator.PublishOnUIThread(new CharacterMessage
    {
        Data = this.Character
    });


    base.OnInitialize();
}

For the child ViewModels where you're only updating the Character data through the EventAggregator, leave the this.eventAggregator.Subscribe(this) call in the constructor.

If any of your child ViewModels are not actually required for the page to function, you could initialize those VM properties via property import:

[Import]
public CharacterGeneralViewModel CharacterGeneralViewModel { get; set; }

Property imports don't occur until after the constructor has completed running.

I would also suggest handling the instantiation of ICharacterSaveService through constructor injection as well, rather than explicitly creating a new instance every time you save data.

The primary purpose of MVVM was to allow front-end designers to work on the layout of the UI in a visual tool (Expression Blend) and coders to implement the behavior and business without interfering with one another. The ViewModel exposes data to be bound to the view, describes the view's behavior at an abstract level, and frequently acts as a mediator to the back-end services.

There is no one "correct" way to do it, and there are situations where it isn't the best solution. There are times when the best solution is to toss the extra layer of abstraction of using a ViewModel and just write some code-behind. So while it's a great structure for your application as a whole, don't fall into the trap of forcing everything to fit into the MVVM pattern. If you have a few more graphically complex user controls where it simply works better to have some code-behind, then that's what you should do.