2
\$\begingroup\$

I’m trying to rewrite one of my projects using the MVVM pattern. I have a feeling though I’ve not done it correctly because I think my Model is too “smart”, it has methods to modify its own datamembers. I’ve seen in examples that people put validation in there but that’s about it. My Model class inherits from INotifyPropertyChanged (allowed?) and contains a dictionary but also methods that manage this dictionary (I left most of them out so it wouldn’t be too long).

In my ModelView class that inherits from INotifyPropertyChanged I have a ObservableCollection of Models (test models, this program is using the .net max SDK to get some of this data I don't think this is important here), this Collection is bound to a listview in my xaml. I have 2 buttons in my View that set properties of my Model.

So basically, where should I put CreateNewMaterialBitArray and SetMaterialIDBit, currently in BaseNode? And is the rest of the code respecting the MVVM pattern?

Model:

public class BaseNode : INotifyPropertyChanged
{
    //Private datamembers
    protected Dictionary<ushort, BitArray> m_MatIDBitArrayDictionary;

    //Properties
    private string m_Name;
    public string Name
    {
        get { return m_Name; }
        set
        {
            m_Name = value;
            InvokePropertyChanged("Name");
        }
    }

    private bool m_Locked;
    public bool Locked
    {
        get { return m_Locked; }
        set
        {
            m_Locked = value;
            InvokePropertyChanged("Locked");
        }
    }

    private bool m_Ignored;
    public bool Ignored
    {
        get { return m_Ignored; }
        set
        {
            m_Ignored = value;
            InvokePropertyChanged("Ignored");
        }
    }

    public event PropertyChangedEventHandler PropertyChanged;
    private void InvokePropertyChanged(string propertyName)
    {
        var e = new PropertyChangedEventArgs(propertyName);
        PropertyChangedEventHandler changed = PropertyChanged;
        if (changed != null) changed(this, e);
    }

    public BaseNode()
    {
        m_MatIDBitArrayDictionary = new Dictionary<ushort, BitArray>();
    }

    //Are these at the right place?
    public void CreateNewMaterialBitArray(ushort matID, int index, int size)
    {
        if (!m_MatIDBitArrayDictionary.ContainsKey(matID))
        {
            var tempBitArray = new BitArray(size);
            tempBitArray.Set(index, true);

            m_MatIDBitArrayDictionary.Add(matID, tempBitArray);
        }
    }

    public void SetMaterialIDBit(ushort matID, int index)
    {
        if (m_MatIDBitArrayDictionary.ContainsKey(matID))
        {
            m_MatIDBitArrayDictionary[matID].Set(index, true);
        }
    }
}

ViewModel:

class BaseNodeViewModel : INotifyPropertyChanged
{
    public ObservableCollection<BaseNode> BaseNodeList{ get; set; }
    public int SelectedBaseNode { get; set; }

    //This isn't really used right now - State of the UI
    private bool m_IsExploded;
    public bool IsExploded
    {
        get { return m_IsExploded; }
        set
        {
            m_IsExploded = value;
            InvokePropertyChanged("IsExploded");
        }
    }

    public BaseNodeViewModel()
    {
        BaseNodeList = new ObservableCollection<BaseNode>();

        //As a test I'll create all the BaseNodes here
        for (uint i = 0; i < 10; i++)
        {
            var newModel = new BaseNode
            {
                Name = "Name_" + i
            };

            //I thought a model had to be "stupid" and only hold data. Can it also modify it?
            newModel.CreateNewMaterialBitArray(5, (int)i, 20);

            BaseNodeList.Add(newModel);
        }
    }

    public void LockSelected()
    {
        BaseNodeList[SelectedBaseNode].Locked = true;
    }

    public void IgnoreSelected()
    {
        BaseNodeList[SelectedBaseNode].Ignored = true;
    }

    public event PropertyChangedEventHandler PropertyChanged;
    private void InvokePropertyChanged(string propertyName)
    {
        var e = new PropertyChangedEventArgs(propertyName);
        PropertyChangedEventHandler changed = PropertyChanged;
        if (changed != null) changed(this, e);
    }
}

View:

public partial class MainWindow : Window
{
    private BaseNodeViewModel m_baseNodeViewModel;

    public MainWindow()
    {
        InitializeComponent();
        m_baseNodeViewModel = new BaseNodeViewModel();
        this.DataContext = m_baseNodeViewModel;
    }

    private void LockSelectedClick(object sender, RoutedEventArgs e)
    {
        m_baseNodeViewModel.LockSelected();
    }

    private void IgnoreSelectedClick(object sender, RoutedEventArgs e)
    {
        m_baseNodeViewModel.IgnoreSelected();
    }
}

xaml:

<StackPanel>
    <ListView ItemsSource="{Binding BaseNodeList}" SelectedIndex="{Binding SelectedBaseNode}">
        <ListView.ItemTemplate>
            <DataTemplate>
                <WrapPanel>
                    <TextBlock Text="{Binding Name}"/>
                    <TextBlock Text=", Locked: " />
                    <TextBlock Text="{Binding Locked}"/>
                    <TextBlock Text=", Ignored: " />
                    <TextBlock Text="{Binding Ignored}"/>
                </WrapPanel>
            </DataTemplate>
        </ListView.ItemTemplate>
    </ListView>
    <Button Content="Lock selected" Click="LockSelectedClick"></Button>
    <Button Content="Ignore selected" Click="IgnoreSelectedClick"></Button>
</StackPanel>
\$\endgroup\$
1
\$\begingroup\$

I have no problem with your model. Your problem is you don't implement commands so you have code in your view which is not ideal (and I'd say wrong). Look into ICommand. Prism has a DelegateCommand class that will create a command that will invoke a private or protected method on your view model and then your button looks like:

<Button Content="Foo" Command="{Binding CommandName}"/>
\$\endgroup\$
  • \$\begingroup\$ Thank you! I read about commands but didn't know why I'd use them when I had the events in my view. Now I do! \$\endgroup\$ – VincentC Mar 25 '15 at 23:09
0
\$\begingroup\$

Some quick remarks:

  • private string m_Name;: I'm not a fan of the m_ naming; convention is an underscore followed by the name in CamelCase, so in this case: _name.

  • InvokePropertyChanged("Name");: Oooh, a magic string. What if you change the property's name and forget to update this? There are solutions that use reflection to fetch the name of the property which prevent such errors.

  • protected Dictionary<ushort, BitArray> m_MatIDBitArrayDictionary;: IMHO anything that isn't private should probably have a getter/setter.

  • tempBitArray: Avoid using names like "temp", since it is meaningless.


WRT this method:

public void CreateNewMaterialBitArray(ushort matID, int index, int size)
{
    if (!m_MatIDBitArrayDictionary.ContainsKey(matID))
    {
        var tempBitArray = new BitArray(size);
        tempBitArray.Set(index, true);

        m_MatIDBitArrayDictionary.Add(matID, tempBitArray);
    }
}

I try to avoid unnecessary indentation, so I'd write:

public void CreateNewMaterialBitArray(ushort matID, int index, int size)
{
    if (m_MatIDBitArrayDictionary.ContainsKey(matID))
    {
        return;
    }

    var tempBitArray = new BitArray(size);
    tempBitArray.Set(index, true);

    m_MatIDBitArrayDictionary.Add(matID, tempBitArray);
}

I also support JasonLind's advice: look into ICommand to bind methods to buttons.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I forgot to change the protected variable to private because in my original (non mvvm) code I inherit from this class. What is the reason behind adding a getter/setter for m_MatIDBitArrayDictionary? \$\endgroup\$ – VincentC Mar 25 '15 at 23:11

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.