7
\$\begingroup\$

I read a lot of articles recently concerning XAML, WPF, data binding and MVVM. I also watched some MVVM tutorials and downloaded some MVVM samples. I finally got my first MVVM sample to work, but there seem to be different "flavors" of MVVM, or maybe there are just many people who get the whole thing totally wrong, I don't know. I tried to do it my way and think it's quite a clean and neat approach. But I've never seen any "real-life" code, which I could compare my code with.

What I try to do is edit a model instance (Gebiet) with simply 2 properties, 1 string (Name) and 1 integer (Nummer). Plus, I wanted the ViewModel to not modify the Model before the "Ok" button is clicked, so I'm internally working on a copy of the model instance. I don't want to use any 3rd party stuff or MVVM frameworks yet, as I would like to fully get the whole concept first before I start trying to make my life easier. And I was insecure what is the best way to handle the int property, as that is sensitive to invalid user input. The way I did it, the TextBox text simply reverts to its previous value on invalid user input.

My ViewModel is quite big for just 2 props, but that seems to be normal!? My biggest problem was to get the commands into the ViewModel, so I don't need to handle the button clicks in my code behind (aside from closing the window on "Ok").

Can you please have a look at my classes and tell me if there's anything I got totally wrong about MVVM? And a suggestion would be nice on how to change the enabled state of the Ok button immediately on any change in the Name TextBox.

Model:

namespace Gebietsmanager
{
    public class Gebiet
    {
        public int Nummer { get; set; }
        public string Name { get; set; }
    }
}

View:

<Window x:Class="Gebietsmanager.GebietBearbeitenDlg.View"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:Gebietsmanager.GebietBearbeitenDlg"
        mc:Ignorable="d"
        d:DataContext="{d:DesignInstance local:ViewModel}"
        Title="Gebiet bearbeiten" Height="142" Width="300" WindowStartupLocation="CenterOwner" ShowInTaskbar="False" ResizeMode="NoResize">
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="Auto"/>
            <RowDefinition Height="Auto"/>
            <RowDefinition Height="Auto"/>
        </Grid.RowDefinitions>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="Auto"/>
            <ColumnDefinition Width="*"/>
        </Grid.ColumnDefinitions>
        <Label Margin="8,8,0,0">Nummer:</Label>
        <TextBox Name="TextBoxNummer" Grid.Row="0" Grid.Column="1" Text="{Binding Nummer}" Margin="8,8,8,0" Width="Auto" GotFocus="TextBox_GotFocus"/>
        <Label Grid.Row="1" Grid.Column="0" Margin="8,8,0,0">Name:</Label>
        <TextBox Name="TextBoxName" Grid.Row="1" Grid.Column="1" Text="{Binding Name}" Margin="8,8,8,0" GotFocus="TextBox_GotFocus"/>
        <StackPanel Grid.Row="2" Grid.ColumnSpan="2" Grid.Column="0" Orientation="Horizontal" Margin="8,8,0,0">
            <Button IsDefault="True" Command="{Binding Commit}" Click="ButtonOk_Click" CommandParameter="{Binding Name}">Ok</Button>
            <Button Command="{Binding Rollback}" Margin="8,0,0,0">Reset</Button>
            <Button IsCancel="True" Margin="8,0,0,0">Cancel</Button>
        </StackPanel>
    </Grid>
</Window>

Code behind:

using System.Windows;
using System.Windows.Controls;

namespace Gebietsmanager.GebietBearbeitenDlg
{
    /// <summary>
    /// Interaktionslogik für View.xaml
    /// </summary>
    public partial class View
    {
        public View()
        {
            InitializeComponent();
        }

        private void TextBox_GotFocus(object sender, RoutedEventArgs e)
        {
            _TextBoxSelectAll(sender as TextBox);
        }

        private void _TextBoxSelectAll(TextBox textBox)
        {
            textBox?.SelectAll();
        }

        private void ButtonOk_Click(object sender, RoutedEventArgs e)
        {
            Close();
        }
    }
}

ViewModel:

using System;
using System.ComponentModel;

namespace Gebietsmanager.GebietBearbeitenDlg
{
    public class ViewModel : INotifyPropertyChanged
    {
        public ViewModel(Gebiet gebiet)
        {
            _gebiet = gebiet;
            _gebietCopy = new Gebiet();
            Helpers.CopyPropValues(_gebietCopy, gebiet);

            Commit = new Command(
                () => Helpers.CopyPropValues(_gebiet, _gebietCopy),
                () => !string.IsNullOrEmpty(_gebietCopy.Name));
            Rollback = new Command(_Rollback);
        }

        private readonly Gebiet _gebiet;
        private readonly Gebiet _gebietCopy;

        private void _Rollback()
        {
            Helpers.CopyPropValues(_gebietCopy, _gebiet);
            OnPropertyChanged();
        }

        public string Nummer
        {
            get { return _gebietCopy.Nummer.ToString(); }
            set
            {
                if (_gebietCopy.Nummer.ToString() != value)
                {
                    try
                    {
                        _gebietCopy.Nummer = Convert.ToInt32(value);
                    }
                    catch (Exception)
                    {
                        // ignored
                    }

                    OnPropertyChanged(nameof(Nummer));
                }
            }
        }

        public string Name
        {
            get { return _gebietCopy.Name; }
            set {
                if (_gebietCopy.Name != value)
                {
                    _gebietCopy.Name = value;
                    OnPropertyChanged(nameof(Name));
                }
            }
        }

        public Command Commit { get; private set; }
        public Command Rollback { get; private set; }

        public event PropertyChangedEventHandler PropertyChanged;
        private void OnPropertyChanged(string propertyName = null)
        {
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));

            if (string.IsNullOrEmpty(propertyName) || propertyName.Equals(nameof(Name)))
            {
                Commit.RaiseCanExecuteChanged();
            }
        }
    }
}

I also wrote myself my own class implementing the ICommand interface, as Microsoft doesn't seem to provide any standard command with Visual Studio!?

using System;
using System.Windows.Input;

namespace Gebietsmanager
{
    public sealed class Command : ICommand
    {
        public Command(Action executeAction, Func<bool> canExecutePredicate = null)
        {
            _executeAction = executeAction;
            _canExecutePredicate = canExecutePredicate;
        }

        private readonly Action _executeAction;
        private readonly Func<bool> _canExecutePredicate;

        public void Execute(object parameter)
        {
            _executeAction?.Invoke();
        }

        public bool CanExecute(object parameter)
        {
            return _canExecutePredicate?.Invoke() ?? true;
        }

        public event EventHandler CanExecuteChanged;

        private void OnCanExecuteChanged()
        {
            CanExecuteChanged?.Invoke(this, EventArgs.Empty);
        }

        public void RaiseCanExecuteChanged()
        {
            OnCanExecuteChanged();
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Your implementation looks fine to me. In real life application, you will have base implementations for INotifyPropertyChanged interface, which will cover some of the most common use cases (firing the event, data validation, commit/rollback scenario). You implement those once, and then simply re-use them in your actual view models.

Also:

  1. You should not use underscores in method names.
  2. The common implementation of ICommand interface uses CommandManager internally. Check it out. This way you won't have to manually call RaiseCanExecuteChanged() on your commands.
  3. You can use int.TryParse method instead of try catch. Also, there are other ways to deal with data convertion. You can implement IValueConverter interface and handle invalid input there. You can implement your own control which would filter the user input for you (I actually posted the code here on CodeReview a few years back). More options will be available as you become more comfortable with WPF. But using strings for properties which require conversion is a good way to start.
  4. You don't have to set CommandParameter if you don't use it.
\$\endgroup\$
  • \$\begingroup\$ 1. Ok 2. Cool! Works fine and makes my code shorter and my life easier. Thank you! 3. IValueConverter is a good idea. I've experimented with this construct a little already. I'll try that next time i have an int value I want to edit. 4. That was just a leftover from debugging, as well as the Name property of the 2 TextBox controls. I removed all these. Are there any scenarios where using a CommandParameter cannot be avoided? Bc I can't think of any!? \$\endgroup\$ – Kim Homann Mar 31 '16 at 14:19
  • \$\begingroup\$ @KimHomann, no, I don't think there are. There are scenarios, where using a parameter can be convinient though. For example, if you have a ListBox with a couple of items, you can bind CommandParameter of your DeleteCommand to ListBox.SelectedItem. You can then access this item in your command via the argument, instead of declaring a SelectedItem property (which you otherwise might not need) on your viewmodel. \$\endgroup\$ – Nikita B Mar 31 '16 at 14:41
  • \$\begingroup\$ This is my first time on codereview.stackexchange.com . Am I supposed to alter my article to show the changes I have made based on your review? I would like to, but then it's not the original question any more, so... \$\endgroup\$ – Kim Homann Apr 1 '16 at 11:15
  • \$\begingroup\$ @KimHomann, you should not change the code in question. You are free to post a follow up question though: meta.codereview.stackexchange.com/a/1066/24349 . You can also answer your own question, if you want to show modified code, but do not want a review: meta.codereview.stackexchange.com/a/1765/24349 \$\endgroup\$ – Nikita B Apr 1 '16 at 13:22

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.