Problem
I’ve coded a calculator in C# with WPF as the UI.
I wish to know mainly about these points:
- Ways of optimizing
- Better techniques, tactics and ways of coding this
- All flaws on the surface as well as deep
- Simpler logic
MainWindow.xaml.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;
namespace NewCalculator
{
public partial class MainWindow : Window
{
static int[] numbersArray = new int[10];
static string[] operatorsArray = new string[9];
static string storageVariable;
static int numbersCounter = 0;
static int operatorsCounter = 0;
static int total = 0;
static bool totalled = false;
public MainWindow()
{
InitializeComponent();
}
private void One_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "1";
storageVariable += "1";
}
private void Two_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "2";
storageVariable += "2";
}
private void Three_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "3";
storageVariable += "3";
}
private void Four_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "4";
storageVariable += "4";
}
private void Five_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "5";
storageVariable += "5";
}
private void Six_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "6";
storageVariable += "6";
}
private void Seven_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "7";
storageVariable += "7";
}
private void Eight_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "8";
storageVariable += "8";
}
private void Nine_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "9";
storageVariable += "9";
}
private void Zero_Click(object sender, RoutedEventArgs e)
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "0";
storageVariable += "0";
}
private void Add_Click(object sender, RoutedEventArgs e)
{
setNumber(storageVariable);
setOperator("+");
Display.Content += "+";
}
private void Subtract_Click(object sender, RoutedEventArgs e)
{
setNumber(storageVariable);
setOperator("-");
Display.Content += "-";
}
private void Multiply_Click(object sender, RoutedEventArgs e)
{
setNumber(storageVariable);
setOperator("*");
Display.Content += "x";
}
private void Divide_Click(object sender, RoutedEventArgs e)
{
setNumber(storageVariable);
setOperator("/");
Display.Content += "/";
}
private void Equal_Click(object sender, RoutedEventArgs e)
{
setNumber(storageVariable);
for (int i = 0; i < operatorsCounter; i++)
{
if (operatorsArray[i] == "+" && i == 0)
{
total = numbersArray[i] + numbersArray[i + 1];
}
else if (operatorsArray[i] == "+")
{
total = total + numbersArray[i + 1];
}
else if (operatorsArray[i] == "-" && i == 0)
{
total = numbersArray[i] - numbersArray[i + 1];
}
else if (operatorsArray[i] == "-")
{
total = total - numbersArray[i + 1];
}
else if (operatorsArray[i] == "*" && i == 0)
{
total = numbersArray[i] * numbersArray[i + 1];
}
else if (operatorsArray[i] == "*")
{
total = total * numbersArray[i + 1];
}
else if (operatorsArray[i] == "/" && i == 0)
{
total = numbersArray[i] / numbersArray[i + 1];
}
else if (operatorsArray[i] == "/")
{
total = total / numbersArray[i + 1];
}
}
Display.Content = total;
numbersArray = null;
operatorsArray = null;
storageVariable = null;
numbersCounter = 0;
operatorsCounter = 0;
total = 0;
totalled = true;
}
static void setNumber(String Number)
{
numbersArray[numbersCounter] = Convert.ToInt16(Number);
storageVariable = null;
numbersCounter++;
}
static void setOperator(String Op)
{
operatorsArray[operatorsCounter] = Op;
operatorsCounter++;
}
private void AC_Click(object sender, RoutedEventArgs e)
{
Display.Content = "";
numbersArray = null;
operatorsArray = null;
storageVariable = null;
numbersCounter = 0;
operatorsCounter = 0;
total = 0;
}
}
}
MainWindow.xaml
<Window x_Class="NewCalculator.MainWindow"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns_x="http://schemas.microsoft.com/winfx/2006/xaml"
Title="Calculator" Height="200" Width="195" ResizeMode="NoResize">
<Grid>
<Label x_Name="Display" Content="" HorizontalAlignment="Left" Height="32" Margin="10,3,0,0" VerticalAlignment="Top" Width="138" BorderThickness="1" RenderTransformOrigin="0.543,1.375" />
<Button x_Name="One" Content="1" HorizontalAlignment="Left" Height="20" Margin="10,40,0,0" VerticalAlignment="Top" Width="20" Click="One_Click"/>
<Button x_Name="Two" Content="2" HorizontalAlignment="Left" Height="20" Margin="40,40,0,0" VerticalAlignment="Top" Width="20" Click="Two_Click"/>
<Button x_Name="Three" Content="3" HorizontalAlignment="Left" Height="20" Margin="70,40,0,0" VerticalAlignment="Top" Width="20" Click="Three_Click"/>
<Button x_Name="Four" Content="4" HorizontalAlignment="Left" Height="20" Margin="10,70,0,0" VerticalAlignment="Top" Width="20" Click="Four_Click"/>
<Button x_Name="Five" Content="5" HorizontalAlignment="Left" Height="20" Margin="40,70,0,0" VerticalAlignment="Top" Width="20" Click="Five_Click"/>
<Button x_Name="Six" Content="6" HorizontalAlignment="Left" Height="20" Margin="70,70,0,0" VerticalAlignment="Top" Width="20" Click="Six_Click"/>
<Button x_Name="Seven" Content="7" HorizontalAlignment="Left" Height="20" Margin="10,100,0,0" VerticalAlignment="Top" Width="20" Click="Seven_Click"/>
<Button x_Name="Eight" Content="8" HorizontalAlignment="Left" Height="20" Margin="40,100,0,0" VerticalAlignment="Top" Width="20" Click="Eight_Click"/>
<Button x_Name="Nine" Content="9" HorizontalAlignment="Left" Height="20" Margin="70,100,0,0" VerticalAlignment="Top" Width="20" Click="Nine_Click"/>
<Button x_Name="Zero" Content="0" HorizontalAlignment="Left" Height="20" Margin="40,130,0,0" VerticalAlignment="Top" Width="20" Click="Zero_Click"/>
<Button x_Name="Add" Content="+" HorizontalAlignment="Left" Height="20" Margin="100,40,0,0" VerticalAlignment="Top" Width="20" Click="Add_Click"/>
<Button x_Name="Subtract" Content="-" HorizontalAlignment="Left" Height="20" Margin="130,40,0,0" VerticalAlignment="Top" Width="20" Click="Subtract_Click"/>
<Button x_Name="Multiply" Content="x" HorizontalAlignment="Left" Height="20" Margin="100,70,0,0" VerticalAlignment="Top" Width="20" Click="Multiply_Click"/>
<Button x_Name="Divide" Content="/" HorizontalAlignment="Left" Height="20" Margin="130,70,0,0" VerticalAlignment="Top" Width="20" Click="Divide_Click"/>
<Button x_Name="Equal" Content="=" HorizontalAlignment="Left" Height="20" Margin="100,100,0,0" VerticalAlignment="Top" Width="20" Click="Equal_Click"/>
<Button x_Name="AC" Content="AC" Height="20" Margin="130,100,17,0" VerticalAlignment="Top" Width="30" Click="AC_Click"/>
</Grid>
</Window>
Solution
Duplication
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "1";
storageVariable += "1";
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += "2";
storageVariable += "2";
All the way up to 0. (1-9 + 0)
If you can somehow figure out which button caused the click event, you can make 1 click function for all your numeric buttons. You can also do this for your +, -, / and * buttons.
Bugs
private void AC_Click(object sender, RoutedEventArgs e)
{
Display.Content = "";
numbersArray = null;//null pointer exception on pressing any of the arithmetic buttons
operatorsArray = null;//null pointer exception on pressing any of the arithmetic buttons
storageVariable = null;//null pointer exception on pressing any of the numeric buttons
numbersCounter = 0;
operatorsCounter = 0;
total = 0;
}
Don’t set to null, that leads to things breaking. AC clears the current calculation on a calculator – so set the values back to the initial values.
static void setNumber(String Number)
{
numbersArray[numbersCounter] = Convert.ToInt16(Number);//no bounds check
storageVariable = null;//possible null pointer when clicking arithmetic button
numbersCounter++;
}
static void setOperator(String Op)
{
operatorsArray[operatorsCounter] = Op;//no bounds check
operatorsCounter++;
}
You’re missing bounds checks. Because you’ve limited the amount of numbers and operations that are allowed, it’s possible to get an out of range exception (or whatever C#’s exception is for going out of an array’s bounds).
Commands
In WPF, one very valuable property is the Command property.
For instance:
<Button
x_Name="One"
Content="1"
HorizontalAlignment="Left"
Height="20"
Margin="10,40,0,0"
VerticalAlignment="Top"
Width="20"
Click="One_Click"
/>
can be changed to:
<Button
x_Name="One"
Content="1"
HorizontalAlignment="Left"
Height="20"
Margin="10,40,0,0"
VerticalAlignment="Top"
Width="20"
Command="ClickCommand"
CommandParameter="1"
/>
Usually, I’d do this with a binding to a viewmodel, but if the command is a member of the code behind, it may still work. Two and onward should only involve changing the parameter.
private ICommand clickCommand; // This will get you a lazily assigned command.
public ICommand ClickCommand { get { return clickCommand ?? (clickCommand = new <WhateverYouNameYourCommandClass>(Click); } }
// In this example, the command takes a delegate of type Action<string>
private void Click(string parameter)
{
int integerValue;
if(int.TryParse(parameter, out integerValue))
{
if (totalled == true)
{
Display.Content = "";
totalled = false;
}
Display.Content += parameter;
storageVariable += parameter;
}
}
From there, you can either write another command for your function buttons or use else ifs. This at least replaces nine whole methods with a single, simpler one.
Note that if the above xaml does not work, what you need to do is change the Command property to this: Command="{Binding RelativeSource={RelativeSource AncestorType={x:Type yourXamlNamespace:MainWindow}}, Path=ClickCommand} or set up your data context properly.
Access Modifier
Normally you want to avoid using static unless you want the field, property or method to be available without instancing. Generally, a static field or property is used for application-wide settings. While static method is usually a stateless helper method, such as : Math.Cos, Int.Parse.
So I would remove the static on these lines, as these are tied the instance of the calculator. It wouldn’t make sense to have 2 instances of the calculator that share the same memory, so that any input on one will be synchronized on the other.
static int[] numbersArray = new int[10];
static string[] operatorsArray = new string[9];
static string storageVariable;
static int numbersCounter = 0;
static int operatorsCounter = 0;
static int total = 0;
static bool totalled = false;
and
static void setNumber(String Number)
{
numbersArray[numbersCounter] = Convert.ToInt16(Number);
storageVariable = null;
numbersCounter++;
}
static void setOperator(String Op)
{
operatorsArray[operatorsCounter] = Op;
operatorsCounter++;
}
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:
FirstNumberis the first number you insert and the number for the resultSecondNumberis the number you want to do operations withGetFunctionreturns 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:
- sorry for the long post, but i really recommend looking at
MVVMbecause 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. - 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. - Create classes to simplify the problem. It is easier to understand your own code when you work with objects like a
Calculatorinstead of a bunch ofints,strings andbools.