Calculator with WPF

Question

I built a calculator in C# with WPF for the UI. I'm still a beginner at WPF and C# in general, and I'm just looking for some constructive criticism.

XAML

<Window x:Class="Calculator.MainWindow"
       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:Calculator"
       mc:Ignorable="d"
       Title="Calculator" Height="350" Width="525">
    <Window.Resources>
        <Style TargetType="Button">
            <Setter Property="Margin" Value="1"/>
            <Setter Property="FontSize" Value="20"/>
            <Setter Property="VerticalContentAlignment" Value="Center"/>
            <Setter Property="FontFamily" Value="Consolas"/>
        </Style>
    </Window.Resources>
    <DockPanel>
        <TextBox DockPanel.Dock="Top" x:Name="currentEquation" Height="50" IsEnabled="False" FontSize="30" VerticalContentAlignment="Center" FontFamily="Consolas" Foreground="Black">

        </TextBox>
        <Grid>
            <Grid.RowDefinitions>
                <RowDefinition Height="*"></RowDefinition>
                <RowDefinition Height="*"></RowDefinition>
                <RowDefinition Height="*"></RowDefinition>
                <RowDefinition Height="*"></RowDefinition>
            </Grid.RowDefinitions>
            <Grid.ColumnDefinitions>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
            </Grid.ColumnDefinitions>
            <Button x:Name="b1" Grid.Column="0" Content="1"/>
            <Button x:Name="b2" Grid.Column="1" Content="2"/>
            <Button x:Name="b3" Grid.Column="2" Content="3"/>
            <Button x:Name="bdelete" Grid.Column="3" Content="Del"/>
            <Button x:Name="bclear" Grid.Column="4" Content="Clear"/>
            <Button x:Name="b4" Grid.Column="0" Grid.Row="1" Content="4"/>
            <Button x:Name="b5" Grid.Column="1" Grid.Row="1" Content="5"/>
            <Button x:Name="b6" Grid.Column="2" Grid.Row="1" Content="6"/>
            <Button x:Name="bplus" Grid.Column="3" Grid.Row="1" Content="+"/>
            <Button x:Name="bminus" Grid.Column="4" Grid.Row="1" Content="-"/>
            <Button x:Name="b7" Grid.Column="0" Grid.Row="2" Content="7"/>
            <Button x:Name="b8" Grid.Column="1" Grid.Row="2" Content="8"/>
            <Button x:Name="b9" Grid.Column="2" Grid.Row="2" Content="9"/>
            <Button x:Name="btimes" Grid.Column="3" Grid.Row="2" Content="×"/>
            <Button x:Name="bdivide" Grid.Column="4" Grid.Row="2" Content="÷"/>
            <Button x:Name="b0" Grid.Column="0" Grid.ColumnSpan="2" Grid.Row="3" Content="0"/>
            <Button x:Name="bdot" Grid.Column="2" Grid.Row="3" Content="."/>
            <Button x:Name="bequals" Grid.Column="3" Grid.ColumnSpan="2" Grid.Row="3" Content="="/>
        </Grid>
    </DockPanel>
</Window>

C#

using System;
using System.Collections.Generic;
using System.Linq;
using System.Windows;
using System.Windows.Controls;

namespace Calculator
{
    public partial class MainWindow: Window
    {
        Calculator c;

        public MainWindow()
        {
            InitializeComponent();
            this.Loaded += OnLoad;
        }

        void OnLoad(object sender, RoutedEventArgs e)
        {
            c = new Calculator(currentEquation);
            AddClickEvents();
        }

        void AddClickEvents()
        {
            b1.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(1);
            };

            b2.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(2);
            };

            b3.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(3);
            };

            b4.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(4);
            };

            b5.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(5);
            };

            b6.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(6);
            };

            b7.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(7);
            };

            b8.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(8);
            };

            b9.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(9);
            };

            b0.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(0);
            };

            bdot.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press('.');
            };

            bplus.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(Calculator.Function.Add);
            };

            bminus.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(Calculator.Function.Minus);
            };

            btimes.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(Calculator.Function.Times);
            };

            bdivide.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(Calculator.Function.Divide);
            };

            bequals.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(Calculator.Function.Equals);
            };

            bdelete.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(Calculator.Function.Delete);
            };

            bclear.Click += (object sender, RoutedEventArgs e) =>
            {
                c.Press(Calculator.Function.Clear);
            };
        }

        private class Calculator
        {
            TextBox tb;

            private static class States
            {
                public static bool hasDot;
                public static bool willReplace;
                public static Function currentFunction;
                public static Dictionary<float, Function> lastNumberAndFunction;

            }

            public enum Function
            {
                Add,
                Minus,
                Times,
                Divide,
                Equals,
                Delete,
                Clear,
                None
            }

            public Calculator(TextBox text)
            {
                tb = text;
                States.hasDot = false;
                States.willReplace = false;
                States.currentFunction = Function.None;
                States.lastNumberAndFunction = new Dictionary<float, Function>();
            }

            public void Press(int number)
            {
                if(States.willReplace)
                {
                    tb.Text = number.ToString();
                    States.willReplace = false;
                    return;
                };

                tb.Text += number;
            }

            public void Press(char c)
            {
                if(c == '.' && !States.hasDot)
                {
                    tb.Text += c;
                    States.hasDot = true;
                }
            }

            public void Press(Function fn)
            {
                switch(fn)
                {
                    case Function.Add:
                    case Function.Minus:
                    case Function.Times:
                    case Function.Divide:
                        States.lastNumberAndFunction.Add(float.Parse(tb.Text), States.currentFunction);
                        States.currentFunction = fn;
                        States.willReplace = true;
                        break;
                    case Function.Equals:
                        States.lastNumberAndFunction.Add(float.Parse(tb.Text), States.currentFunction);
                        Equals();
                        break;
                    case Function.Delete:
                        tb.Text = String.Join("", tb.Text.Reverse().Skip(1).Reverse());
                        if(!tb.Text.Contains(".")) States.hasDot = false;
                        break;
                    case Function.Clear:
                        tb.Text = "";
                        States.hasDot = false;
                        break;
                }
            }

            void Equals()
            {
                float final = 0;
                foreach(KeyValuePair<float, Function> numberAndFunction in States.lastNumberAndFunction)
                {
                    switch(numberAndFunction.Value)
                    {
                        case Function.None:
                            final = numberAndFunction.Key;
                            break;
                        case Function.Add:
                            final += numberAndFunction.Key;
                            break;
                        case Function.Minus:
                            final -= numberAndFunction.Key;
                            break;
                        case Function.Times:
                            final *= numberAndFunction.Key;
                            break;
                        case Function.Divide:
                            if(numberAndFunction.Key == 0)
                            {
                                final = 0;
                                break;
                            }

                            final /= numberAndFunction.Key;
                            break;
                    }
                }

                tb.Text = final.ToString();
                States.lastNumberAndFunction.Clear();
                States.hasDot = false;
                States.currentFunction = Function.None;
                States.willReplace = true;
            }
        }
    }
}

Very minor suggestion: anytime you have an enum that includes a None value, I'd recommend that None is the first value. — Rick Davin
– Rick Davin, Commented Aug 18, 2015 at 12:36

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

The main problem with your code is that it is hard to test. The view is tightly coupled to the logic of calculating.

I would like to start with recommending the MVVM pattern for any WPF application. It has a good separation of the View and the ViewModel and makes your code more testable. I would like to recommend the Prism library for helping with the MVVM pattern.

I would try to avoid the static keyword as it makes your code harder to test.

Check out this demo project for MVVM in prism: https://code.msdn.microsoft.com/windowsdesktop/MVVM-Code-Sample-using-the-7e59b7bb

Edit

I also found a bug in your solution. If you do a operation on the same number for example 5 + 5 your application throws an error. That is because you are using a dictionary and it can't contain the same key twice. If you would have written your app in MVVM you could unit test your ViewModel.

I would also use decimal instead of float. It is better with precision. https://stackoverflow.com/questions/6341855/how-to-decide-what-to-use-double-or-decimal

Edit 2 - MVVM demo to help you get started

Create a new Wpf application
Add the NuGet package Prism.MVVM
Use dependency injection for testability and program against interfaces.
Delete this StartupUri="MainWindow.xaml" from from App.xaml
Add code below to help you get started

Demo Code - not complete:

public partial class App : Application
{
    protected override void OnStartup(StartupEventArgs e)
    {
        var window = new CalculatorWindow(new CalculatorViewModel(new Calculator()));
        window.Show();
    }
}

public class CalculatorViewModel : BindableBase
{
    private readonly Calculator calculator;
    private string currentValue;

    public CalculatorViewModel(Calculator calculator)
    {
        this.calculator = calculator;
    }

    public string CurrentValue
    {
        get { return currentValue; }
        set
        {
            currentValue = value;
            OnPropertyChanged(() => CurrentValue);
        }
    }

    public ICommand OneCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(1)); }
    }

    public ICommand TwoCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(2)); }
    }

    public ICommand ThreeCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(3)); }
    }

    public ICommand FourCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(4)); }
    }

    public ICommand FiveCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(5)); }
    }

    public ICommand SixCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(6)); }
    }

    public ICommand SevenCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(7)); }
    }

    public ICommand EightCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(8)); }
    }

    public ICommand NineCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(9)); }
    }

    public ICommand ZeroCommand
    {
        get { return new DelegateCommand(() => AddNumberToInput(0)); }
    }

    public ICommand DotCommand
    {
        get { return new DelegateCommand(AddDotToInput); }
    }

    public ICommand PlusCommand
    {
        get
        {
            return new DelegateCommand(() =>
            {
                calculator.AddNumber(double.Parse(CurrentValue));
                calculator.AddOperation(Operation.Plus);
            });
        }
    }
    public ICommand MinusCommand { get; private set; }
    public ICommand MultiplyCommand { get; private set; }
    public ICommand DevideCommand { get; private set; }
    public ICommand EqualsCommand { get; private set; }

    public ICommand DeleteCommand { get; private set; }
    public ICommand ClearCommand { get; private set; }

    private void AddNumberToInput(int number)
    {
        CurrentValue += number;
    }

    private void AddDotToInput()
    {
        CurrentValue += ".";
    }
}

<Window x:Class="MVVMCalculator.CalculatorWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="Calculator" Height="350" Width="350">

    <Window.Resources>
        <Style TargetType="Button">
            <Setter Property="Margin" Value="1"/>
            <Setter Property="FontSize" Value="20"/>
            <Setter Property="VerticalContentAlignment" Value="Center"/>
            <Setter Property="FontFamily" Value="Consolas"/>
        </Style>
    </Window.Resources>
    <DockPanel>
        <TextBox DockPanel.Dock="Top" Text="{Binding CurrentValue}" Height="50" 
                 IsEnabled="False" FontSize="30" VerticalContentAlignment="Center" 
                 FontFamily="Consolas" Foreground="Black"/>
        <Grid>
            <Grid.RowDefinitions>
                <RowDefinition Height="*"></RowDefinition>
                <RowDefinition Height="*"></RowDefinition>
                <RowDefinition Height="*"></RowDefinition>
                <RowDefinition Height="*"></RowDefinition>
            </Grid.RowDefinitions>

            <Grid.ColumnDefinitions>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
                <ColumnDefinition Width="*"></ColumnDefinition>
            </Grid.ColumnDefinitions>

            <Button Command="{Binding OneCommand}" Grid.Column="0" Content="1"/>
            <Button Command="{Binding TwoCommand}" Grid.Column="1" Content="2"/>
            <Button Command="{Binding ThreeCommand}" Grid.Column="2" Content="3"/>
            <Button Command="{Binding DeleteCommand}" Grid.Column="3" Content="Del"/>
            <Button Command="{Binding ClearCommand}" Grid.Column="4" Content="Clear"/>
            <Button Command="{Binding FourCommand}" Grid.Column="0" Grid.Row="1" Content="4"/>
            <Button Command="{Binding FiveCommand}" Grid.Column="1" Grid.Row="1" Content="5"/>
            <Button Command="{Binding SixCommand}" Grid.Column="2" Grid.Row="1" Content="6"/>
            <Button Command="{Binding PlusCommand}" Grid.Column="3" Grid.Row="1" Content="+"/>
            <Button Command="{Binding MinusCommand}" Grid.Column="4" Grid.Row="1" Content="-"/>
            <Button Command="{Binding SevenCommand}" Grid.Column="0" Grid.Row="2" Content="7"/>
            <Button Command="{Binding EightCommand}" Grid.Column="1" Grid.Row="2" Content="8"/>
            <Button Command="{Binding NineCommand}" Grid.Column="2" Grid.Row="2" Content="9"/>
            <Button Command="{Binding MultiplyCommand}" Grid.Column="3" Grid.Row="2" Content="×"/>
            <Button Command="{Binding DevideCommand}" Grid.Column="4" Grid.Row="2" Content="÷"/>
            <Button Command="{Binding ZeroCommand}" Grid.Column="0" Grid.ColumnSpan="2" Grid.Row="3" Content="0"/>
            <Button Command="{Binding DotCommand}" Grid.Column="2" Grid.Row="3" Content="."/>
            <Button Command="{Binding EqualsCommand}" Grid.Column="3" Grid.ColumnSpan="2" Grid.Row="3" Content="="/>
        </Grid>
    </DockPanel>
</Window>

Thank you. So instead of using static I should use instances? For example my States class, which holds a bunch of states, I should have an instance instead of static? — hexagonest
– hexagonest, Commented Aug 15, 2015 at 10:45
Yes you lose nothing by losing the static keyword and having an instance veriable. You can implemenmt MVVM without prism and I have. You can also find MVVM project templates which can help you. But I recommend prism at least for the commands and notify property changed. — Jakob
– Jakob, Commented Aug 15, 2015 at 11:24
static is not wrong to use by any means, and in many cases using static is an easy way to find code that should be encapsulated into a class. What i'm more or less saying is use static until you have enough similar code to group into a class then use it accordingly. (that would make that group of methods non-static) which could be summed up as a TODO statement :) — Robert Snyder
– Robert Snyder, Commented Aug 15, 2015 at 20:31

Robert Snyder · Accepted Answer · 2015-08-15 21:02:45Z

I very much agree with Jakob about using MVVM as a pattern of choice with WPF. The two go very well hand in hand. Since Jakob mentioned it, this is as much as I will mention it. As your your code there are a few things you can do to clean up a few lines of code.

b1.Click += (object sender, RoutedEventArgs e) =>
{
    c.Press(1);
};

in C# using lambdas is nice and easy. There are also a few shortcuts. You don't have to specify the type and if you don't plan on using the variables from the event naming them _ and __ implies that you don't care about them. this would make it short enough to inline it

b1.Click += _,__ => c.Press(1);
b2.Click += _,__ => c.Press(2);
b3.Click += _,__ => c.Press(3);
b4.Click += _,__ => c.Press(4);
b5.Click += _,__ => c.Press(5);
b6.Click += _,__ => c.Press(6);

any single letter variables are bad name choices. (with the exception of i, j, and k used in for loops, but even there sometimes it is better to not use a single character variable name)

you could have made Calculator public and in its own separate file. As Jakob mentioned it would be for testing purposes. Why bring up unit testing in two (potentially more answers) because of how useful it is. There are a few frameworks available, both have their pro's and con's, but as a whole they should not be ignored.There are tons of blogs, posts, video's, tutorials out on the net to help you get started, but I will give a simple example to show the error that Jakob found

so lets pretend that I made Calculator public, and removed the dependencies on anything WPF (IE... the TextBox)

[Test]
public void WhenTwoOfTheSameNumbersAreAdded_ReturnedResultIsDouble()
{
    var calculator = new Calculator(null);
    var result = 0f;
    calculator.ResultCalculated += value => result = value;

    calculator.Press(5);
    calculator.Press(Calculator.Function.Add);
    calculator.Press(5);
    calculator.Press(Calculator.Function.Equals);

    Assert.That(result, Is.EqualTo(10f));
}

I would have made an event of type Action<float> (or possibly decimal if i needed more precise calculations) that returns the result of a calculation. With this simple test in place I can see that I have a bug in the code as was suggested with Jakob, and I will also know very quickly when the bug gets fixed because this test will pass. (it takes on my machine including build time) about 500ms to run the above code. after it is built, though, I can run that test in less then 1ms.

I have mixed feelings about the enum being declared in Calculator. On one hand it does read nicely... (With the minor exception of I would prefer to make it plural Functions instead)... On the other hand it is nested. A nice compromise maybe would be to extract it to its own file with the name CalculatorFunctions.

The XAML of your code looks nice. There could be some minor tweaking to it to make it easier to scale to different resolutions and screen sizes, but this does look ok.

Regarding enum being named plural: Microsoft recommends that if the enumeration defines distinct, simple values that the name should be singular, but if the enum is defined with flags that it should be plural. — Rick Davin
– Rick Davin, Commented Aug 18, 2015 at 12:41

Stack Exchange Network

Calculator with WPF

2 Answers 2

Edit

Edit 2 - MVVM demo to help you get started

You must log in to answer this question.

Hot Network Questions

Calculator with WPF

2 Answers 2

Edit

Edit 2 - MVVM demo to help you get started

You must log in to answer this question.

Related

Hot Network Questions