Return to Revisions

The View (XAML)

First i want to discuss the View because the Code is not really good readable. It is a huge block of code (which is not bad) but it has stairs in it which looks very ugly. Try to insert some whitespaces to solve this. This makes only sense when the difference of the spacing is very low. when you have to add 20 white spaces this may not be a great solution.

Another thing is the Alignment of the Buttons and Labels. You should not use Margin for positioning. Grid has a Grid.RowDefinition and Grid.ColumnDefinition Property which i also described in this Code Review but i will also post my Calculator code in the bottom.

Like some others i would also recommend using MVVM. It is a bit more programming effort but it is totally worth it because it makes your code more maintainable.

The XAML Layout looks like this with applied MVVM patterns, it is very long (and wide) but very good readable:

<Window x:Class="Calculator.MainWindow"
 xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
 xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
 Title="MainWindow" Height="350" Width="525"
 Name="CalculatorWindow"
 DataContext="{StaticResource ViewModel}">
 <Grid>
 <Grid.RowDefinitions>
 <RowDefinition Height="60" />
 <RowDefinition Height="60" />
 <RowDefinition Height="*" />
 </Grid.RowDefinitions>
 <Label Grid.Row="0" Content="{Binding Calculator.FirstNumber}" HorizontalContentAlignment="Center" VerticalAlignment="Center" FontSize="40"/>
 <Label Grid.Row="1" Content="{Binding Calculator.SecondNumber}" HorizontalContentAlignment="Center" VerticalAlignment="Center" FontSize="40"/>
 <Grid Grid.Row="2">
 <Grid.RowDefinitions>
 <RowDefinition />
 <RowDefinition />
 <RowDefinition />
 <RowDefinition />
 <RowDefinition />
 </Grid.RowDefinitions>
 <Grid.ColumnDefinitions>
 <ColumnDefinition />
 <ColumnDefinition />
 <ColumnDefinition />
 <ColumnDefinition />
 <ColumnDefinition />
 </Grid.ColumnDefinitions>
 <Button Content="MC" Grid.Column="0" Grid.Row="0"/>
 <Button Content="MR" Grid.Column="1" Grid.Row="0"/>
 <Button Content="MS" Grid.Column="2" Grid.Row="0"/>
 <Button Content="AC" Grid.Column="4" Grid.Row="0"/>
 <Button Content="1" Grid.Column="0" Grid.Row="1" Command="{Binding NumberCommand}" CommandParameter="1"/>
 <Button Content="2" Grid.Column="1" Grid.Row="1" Command="{Binding NumberCommand}" CommandParameter="2"/>
 <Button Content="3" Grid.Column="2" Grid.Row="1" Command="{Binding NumberCommand}" CommandParameter="3"/>
 <Button Content="4" Grid.Column="0" Grid.Row="2" Command="{Binding NumberCommand}" CommandParameter="4"/>
 <Button Content="5" Grid.Column="1" Grid.Row="2" Command="{Binding NumberCommand}" CommandParameter="5"/>
 <Button Content="6" Grid.Column="2" Grid.Row="2" Command="{Binding NumberCommand}" CommandParameter="6"/>
 <Button Content="7" Grid.Column="0" Grid.Row="3" Command="{Binding NumberCommand}" CommandParameter="7"/>
 <Button Content="8" Grid.Column="1" Grid.Row="3" Command="{Binding NumberCommand}" CommandParameter="8"/>
 <Button Content="9" Grid.Column="2" Grid.Row="3" Command="{Binding NumberCommand}" CommandParameter="9"/>
 <Button Content="0" Grid.Column="1" Grid.Row="4" Command="{Binding NumberCommand}" CommandParameter="0"/>
 <Button Content="," Grid.Column="2" Grid.Row="4"/>
 <Button Content="+" Grid.Column="3" Grid.Row="1" Command="{Binding OperationCommand}" CommandParameter="+" />
 <Button Content="-" Grid.Column="4" Grid.Row="1" Command="{Binding OperationCommand}" CommandParameter="-"/>
 <Button Content="x" Grid.Column="3" Grid.Row="2" Command="{Binding OperationCommand}" CommandParameter="*"/>
 <Button Content="/" Grid.Column="4" Grid.Row="2" Command="{Binding OperationCommand}" CommandParameter="/"/>
 <Button Content="%" Grid.Column="3" Grid.Row="3" Command="{Binding OperationCommand}" CommandParameter="%"/>
 <Button Content="mod" Grid.Column="4" Grid.Row="3" Command="{Binding OperationCommand}" CommandParameter="mod"/>
 <Button Content="=" Grid.Column="4" Grid.Row="4" Command="{Binding OperationCommand}" CommandParameter="="/>
 </Grid>
 </Grid>
</Window>

Calculator (.cs):

I am not really happy with your calculator, because you reading the numbers as string and always parse them when you do calculations. Because of that i would strongly recommend to create a class which handles the numbers and the calculations.

I have implemented a Calculator with a History so i need two number (and two labels) and an delegate to save the selected Operation.

This is what i have implemented:

• FirstNumber is the first number you insert and the number for the result
• SecondNumber is the number you want to do operations with
• GetFunction returns a delegate to a method. the parameter is a operation like "+" or "*"
  

public class Calculator
{
 public int FirstNumber { get; set; }
 public int SecondNumber { get; set; }
 private Func<int, int, int> Function { get; set; }
 private static Func<int, int, int> GetFunction(string identifier)
 {
 switch (identifier)
 {
 case "+":
 return (a, b) => a + b;
 case "-":
 return (a, b) => a - b;
 case "*":
 return (a, b) => a * b;
 case "/":
 return (a, b) => a / b;
 case "mod":
 return (a, b) => a % b;
 default:
 throw new ArgumentException("Invalid Operation");
 }
 }
 private void ExecuteFunction()
 {
 this.FirstNumber = this.Function(this.FirstNumber, this.SecondNumber);
 this.SecondNumber = 0;
 }
 public void SetOperation(string operation)
 {
 if (operation == "=")
 {
 this.ExecuteFunction();
 this.Function = null;
 return;
 }
 else if (this.Function != null)
 {
 this.ExecuteFunction();
 }
 this.Function = GetFunction(operation);
 }
 public void ExpandNumber(char number)
 {
 var newNumber = number - '0';
 if (this.Function != null)
 {
 this.SecondNumber = this.SecondNumber * 10 + newNumber;
 }
 else
 {
 this.FirstNumber = this.FirstNumber * 10 + newNumber;
 }
 }
}

The ViewModel:

The ViewModel is the "gabcloser" between the View and the Model and contains the Logic. The idea of this is that the code is independent of the UI. that means the UI designer just needs to know which Commands he must call and he can work with your code but do not need to know much about it.

public class ViewModel: NotifyPropertyChangedClass
{
 public Calculator Calculator { get; set; }
 public View()
 {
 this.Calculator = new Calculator ();
 }
 private ICommand numberCommand;
 public ICommand NumberCommand
 {
 get
 {
 return this.numberCommand
 ?? (this.numberCommand = new RelayCommand<string>(this.NumberClicked));
 }
 }
 private ICommand operationCommand;
 public ICommand OperationCommand
 {
 get
 {
 return this.operationCommand
 ?? (this.operationCommand = new RelayCommand<string>(this.OperationClicked));
 }
 }
 private void OperationClicked(string operation)
 {
 this.Calculator.SetOperation(operation);
 this.OnPropertyChanged("Calculator");
 }
 private void NumberClicked(string number)
 {
 this.Calculator.ExpandNumber(number[0]);
 this.OnPropertyChanged("Calculator");
 }
}

and NotifyPropertyChangedClass is this little helper class:

public abstract class NotifyPropertyChangedClass : INotifyPropertyChanged
{
 public event PropertyChangedEventHandler PropertyChanged;
 [NotifyPropertyChangedInvocator]
 protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
 {
 PropertyChangedEventHandler handler = PropertyChanged;
 if (handler != null)
 {
 handler(this, new PropertyChangedEventArgs(propertyName));
 }
 }
}

Relay command is copied 1:1 from this accepted answer and works very good.

Instance of the ViewModel:

If you want to use the above XAML code you need to change the App.xaml to this:

<Application x:Class="Calculator.App"
 xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
 xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
 xmlns:calculator="clr-namespace:Calculator"
 StartupUri="MainWindow.xaml">
 <Application.Resources>
 <calculator:View x:Key="ViewModel" />
 </Application.Resources>
</Application>

Conclusion:

1. sorry for the long post, but i really recommend looking at MVVM because it is awesome. I had two days problems with it but now i work in every project with it because it is so much easier to code with this pattern, because you have not this big EventHandlers everywhere in your View classes.
2. Code with the thought: "Can i still understand my code in 2 months". if not maybe delete a big part of it and try again. it is totally worth it.
3. Create classes to simplify the problem. It is easier to understand your own code when you work with objects like a Calculator instead of a bunch of ints, strings and bools.