WPF Calculator Code

Question

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>

If you changed from using events to using commands, you could include a specific parameter, which means they could all share the same method. That alone might clean things up a lot. — Magus, Jul 23, 2014 at 14:46
In this question, all the answers are correct. The one I ticked is because he is talking about 2 topics while others 1. — Abandoned Account, Jul 24, 2014 at 6:48

Pimgd · Accepted Answer · 2014-07-23 15:03:39Z

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).

Magus · Accepted Answer · 2014-07-23 21:06:41Z

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.

This is what I meant - if you can figure out how to pass a parameter per button, you can make all the buttons use the same function! — Pimgd, Jul 24, 2014 at 7:34
@Pimgd: Yeah, I figured that if the other two answers were specific but left this open, I'd better fill it. Commands are very useful. I'm slightly spoiled because I'm used to the Prism DelegateCommand which supports a second delegate which enables or disables the button. — Magus, Jul 24, 2014 at 14:38

Xiaoy312 · Accepted Answer · 2014-07-23 19:25:14Z

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++;
    }

Community · Accepted Answer · 2017-05-23 12:40:52Z

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:

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.
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 Calculator instead of a bunch of ints, strings and bools.

Stack Exchange Network

WPF Calculator Code

4 Answers 4

Commands

Access Modifier

Your Answer

Not the answer you're looking for? Browse other questions tagged
c#
wpf
calculator
or ask your own question.

Linked

Hot Network Questions

WPF Calculator Code

4 Answers 4

Commands

Access Modifier

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c#wpfcalculator or ask your own question.

Linked

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
c#
wpf
calculator
or ask your own question.