5
\$\begingroup\$

I am currently 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.

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.

\$\endgroup\$
5
\$\begingroup\$

Comments

I need to comment on ...your comments. Really. NONE of the comments you have here are useful.

None of them.


Xml comments

This is rather unusual:

/// <summary>ViewModel for the character view.</summary>

Normally XML comments looks like this:

/// <summary>
/// ViewModel for the character view.
/// </summary>

That's also how the IDE sets them up automatically for you when you type /// (assuming Visual Studio), so having them on 1 line seems like you're fighting your IDE. Embrace it!

The purpose of XML comments is to provide documentation, specifically for the public API. This:

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

...is overkill useless clutter. If you need a comment that says "The event aggregator" for a private field called eventAggregator of type IEventAggregator, ... same here:

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

Banner comments

This is rather annoying:

// --------------------------------------------------------------------------------------------------------------------
// Fields
// -------------------------------------------------------------------------------------------------------------------

Kill it, with fire. Burn. The maintainer that cannot tell a field from a property shouldn't be maintaining your code. The only thing worse you could have done, is wrap them in a #region Fields.


This

I find the this qualifier adds to the clutter, although here it's needed:

public CharacterViewModel(IEventAggregator eventAggregator)
    : this()
{
    this.eventAggregator = eventAggregator;
    this.eventAggregator.Subscribe(this);
}

...but only because eventAggregator isn't called _eventAggregator. All other instances of this can be removed.

That said...


Constructor chaining

You have it reversed: the parameterless/default constructor should be calling the parameterized constructor, not the other way around:

public CharacterViewModel()
    : this(null)
{
    // ...
}

Why are you injecting an IEventAggregator (which is good), but instantiating CharacterGeneralViewModel and CharacterMetadataViewModel? They could be constructor-injected as well.

\$\endgroup\$
  • 1
    \$\begingroup\$ Regarding the usage of this. Actually Stylecop and ReSharper tell me to use this. Since I personally feel it makes the code more readable and according to [stackoverflow.com/questions/449324/… (this) SO Question it makes no difference I will keep it. As for contructor chaining: I always thought you chained the constructor from more to less. Is there any reason to it the other way around? \$\endgroup\$ – Ruhrpottpatriot Sep 4 '14 at 7:15
  • \$\begingroup\$ To this or not to this is really more of a preference, I should have pointed out that I like your consistency here. As for constructors, chaining so as to always end up calling that with the most parameters allows you to move more code to a single constructor, and makes the defaults more obvious (you have an implicit null default with the parameterless constructor); it's all about maintainability ;) \$\endgroup\$ – Mathieu Guindon Sep 4 '14 at 13:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.