Gfrktyl

I'm new to MVVM and WPF. Please suggest whether this is okay or I need to correct my understanding of MVVM which I confess is very limited as at the moment.

I have created a simple addition application which gets two number as input and provide the added number after clicking the Button.

Please give me your honest (and brutal) opinion, since that will help me improve the most.

Model.cs:

using System;

using System.Collections.Generic;

using System.Linq;

using System.Text;

using System.ComponentModel;



namespace addition.Model

{

    class Number

    {

  public int number1



           { get; set; }





  public int number2

        { get; set; }



  public int number3

        { get; set; }





    }

}

ViewModel.cs

using addition.Model;



using System;

using System.Collections.Generic;

using System.Linq;

using System.Text;

using System.ComponentModel;

using System.Collections.ObjectModel;

using System.Windows;

using System.Windows.Controls;

using System.Windows.Input;





namespace addition.ViewModel

{

    class ViewModel : INotifyPropertyChanged

    {

        private Number n1 = new Number();

        int num, num1;





        public RelayCommand AddNew { get; set; }



       private string _number1;



public string FirstArgument

{



    get { return this._number1; }

    set

    {

        this._number1 = value;

        if (int.TryParse(_number1.ToString(), out num))

        {

            this.n1.number1 = num;

            this.OnPropertyChanged("FirstArgument");



        }

        else { MessageBox.Show("The given Value is not a Number "); }



    }

}

private string _number2;



        public string secondargument

        {

            get { return this._number2; }



            set

            {

                this._number2 = value;

                if (int.TryParse(_number2.ToString(), out num1))

                {

                    this.n1.number2 = num1;

                    this.OnPropertyChanged("secondargument");

                }

                else { MessageBox.Show("The given Value is not a Number "); }



            }

        }



        private string _number3;



        public string Addedargument

        {

            get { return this._number3; }

            set

            {

                this._number3 = value;

                this.OnPropertyChanged("Addedargument");

            }

        }



        public  ViewModel()

    {



        AddNew = new RelayCommand(o => AddNumbers());



    }





    private void AddNumbers()

    {

        var a = this.FirstArgument;

        var b = this.secondargument ;

    var c = (Convert.ToInt32(a) + Convert.ToInt32(b)).ToString();

    MessageBox.Show(c);

    Addedargument = c;





    }



    #region INotifyPropertyChanged Members



    public event PropertyChangedEventHandler PropertyChanged;

    private void OnPropertyChanged(string propertyName)

    {

        if (PropertyChanged != null)

        {

            PropertyChanged(this, new PropertyChangedEventArgs(propertyName));

        }

    }

    #endregion



    }

}

View.xaml

<Window x:Class="addition.Window1"

         xmlns:vm="clr-namespace:addition.ViewModel" 

    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"

    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"

    Title="Window1" Height="300" Width="300">

    <Window.DataContext>

        <vm:ViewModel />

    </Window.DataContext>

    <Grid>



        <Label Height="28" Margin="28,54,0,0" Name="Number1" VerticalAlignment="Top" HorizontalAlignment="Left" Width="48">Number</Label>

        <TextBox Height="28" Margin="112,56,46,0"  Text ="{Binding Path = FirstArgument}"   Name="textBox1" VerticalAlignment="Top" />

        <Label Margin="28,106,0,128" Name="Number2" Width="58" HorizontalAlignment="Left">Number1</Label>

        <TextBox Height="28" Margin="112,120,46,120" Text ="{Binding  Path = secondargument}" Name="textBox2" />

        <Label Height="28" Margin="28,0,0,75" Name="label1" VerticalAlignment="Bottom" HorizontalAlignment="Left" Width="58">Number2</Label>

        <TextBox Height="23" Margin="112,0,46,68" Name="textBox3" Text="{Binding Path = Addedargument}" VerticalAlignment="Bottom" />

        <Button Height="23"  HorizontalAlignment="Left" Margin="39,0,0,16" Name="button1" VerticalAlignment="Bottom" Width="75" Command="{Binding AddNew}">Button</Button>

    </Grid>

</Window>

Some quick remarks:

• Give things proper names. "Model.cs" is a bad name for a class, and in your case it is even the name of a namespace. Microsoft has Naming Guidelines; please follow them.




• Same for properties, e.g. public int number1: follow Microsoft's Naming Guidelines.




• Give things meaningful names. You're not improving your code by obscuring your names of fields, variables etc., e.g. n1, num, num1.




• Why are you doing _number1.ToString(), when _number1 is already a string?




• Be consistent in naming: FirstArgument is correctly named, yet secondargument makes two mistakes against the guidelines. And then Addedargument makes one mistake against the guidelines.




• Why are those "arguments" strings and not ints? You check this in their gets yet store them as strings, causing you to again needing to convert them in AddNumbers().




• Use a Grid or a StackPanel for lay-outs instead of placing items via defined margins.




• Use nameof() instead of a "magic string" in this.OnPropertyChanged("FirstArgument");.




• Don't use a MessageBox in your ViewModel. Look at the approaches discussed in this StackOverflow question for better solutions.




• Avoid clutter in your XAML. It's been a while since I've done such development, but IIRC you don't need to give everything a Name. Communication between Labels and TextBoxes and the VM should be done via Bindings, and thus names are not needed.




• Give your button a proper text. "Button" is stating the obvious and doesn't explain to the user what it does.

To end on a compliment: you're using Bindings and Commands, which is excellent.