HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

C# WPF Simple Calculator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
wpfsimplecalculator

Problem

I've implemented simple +-*/ calculator in WPF. I know that here are few implementations of the same thing, but each is different, with different advantages or disadvantages against this one. Can you suggest me, what could be improved in my code?

```
// calculator operations
public enum CalcOperator
{
None,
Plus,
Minus,
Times,
Divide
}

public partial class Calculator : Window
{
// decimal separator of current culture
char decimSepar = Convert.ToChar
(CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator);

private double a = 0; // the first number
private double b = 0; // second number

// last selected operation
private CalcOperator lastOper = CalcOperator.None;

public Calculator()
{
InitializeComponent();
}

// handle single digit (button)
public void HandleDigit(int i)
{
string str = txtDisp.Text;

if (lastOper == CalcOperator.None && a != Convert.ToDouble(str) ||
lastOper != CalcOperator.None && b != Convert.ToDouble(str) ||
str == "0")
{
str = string.Empty;
}

str += i.ToString();

if (lastOper == CalcOperator.None)
a = Convert.ToDouble(str);
else
b = Convert.ToDouble(str);

txtDisp.Text = str;
}

// handle calculator operation (button)
public void HandleOperator(CalcOperator oper)
{
txtDisp.Text = b.ToString();
lastOper = oper;
}

// handle decimal separator selection (button)
public void HandleDecimal()
{
if (!txtDisp.Text.Contains(decimSepar))
txtDisp.Text += decimSepar;
}

// compute the result
public void Compute()
{
double result = 0.0;

switch (lastOper)
{
case CalcOperator.Plus:
result = a + b;
break;
case CalcOperator.Minus:
result = a - b;

Solution

I'm glad to see someone did a WPF calculator, and I hope that I can add to what was already stated. Before I get started: "yes this is a MVVM rant". Now that I got that out of the way, I can understand though why a person would do a code behind approach. One is that it is what a ton of people are used too (I was one of them coming from Java, then going to winforms and eventually landing on WPF) Two is that at first glance it looks to be more compact and easier to understand. So why should a person do it? Well for one is that you'll be less tempted to put FrameworkElement properties in your code. string str = txtDisp.Text;, txtDisp.Text = str;, txtDisp.Text = b.ToString(); ...etc

Why is that helpful? well if you were to write tests for your code to make sure your logic is sound you don't want to setup a window in your favorite test framework. (trust me..I tried) However if you setup your ViewModel using normal primitive properties and bind your Window to those properties you'll find it much easier to test. I can imagine my ViewModel looking sorta like this

public class CalculatorViewModel : INotifyPropertyChanged
{
    public string Display {get; private set;}

    public ICommand NumericButtonPressedCommand {get; private set;}

    //... other things to set this all up

    //called from the UI and state contains a string of 0-9
    private void NumericButtonPressed(object state)
    {
        //convert state to int, update display
    }
}


TIP you can find much more information about MVVM and how to properly bind and setup commands all over the internet.

The viewmodel above can easily be tested with something like this:

public class CalculatorViewModelTest
{
    [Test]
    public void WhenNumericButtonIsPressedDigitIsShownOnDisplay()
    {
        var viewModel = new CalculatorViewModel();

        NumericButtonPressedCommand.Execute("4");

        Assert.That(viewModel.Display, Is.EqualTo("4"));

        NumericButtonPressedCommand.Execute("4");
        NumericButtonPressedCommand.Execute("2");
        NumericButtonPressedCommand.Execute("4");

        Assert.That(viewModel.Display, Is.EqualTo("4,242"));
    }
}


This is another reason why it's good to separate your logic from your UI. It facilitates easier testing.

So enough of a rant about that. If you decide to give it a go you'll read that over and over and over again. Your code uses the Tag object property of your buttons which IMO is much less clear than just setting up 4 (or more) operator ClickListeners. If you were to revisit this code in a few days or weeks and say your plus button was subtracting. You'd first look at Compute and see that it looks for a CalcOperator being set to lastOper. You look to see where it is set which is by HandleOperator, that is called from OnClickOperator and uses a Tag. Now you are searching for where Tag is set and you finally see that you accidentally set the Plus button's Tag to Subtract. copy paste error. Had you had individual Operator ClickListeners you would probably have gone to the OnClickAddition and see that you accidently set lastOper to Subtract. (In reality you didn't do that, but you can see how tracking the bug is easier when some things are spelled out. Not always, but sometimes)

Code Snippets

public class CalculatorViewModel : INotifyPropertyChanged
{
    public string Display {get; private set;}

    public ICommand NumericButtonPressedCommand {get; private set;}

    //... other things to set this all up

    //called from the UI and state contains a string of 0-9
    private void NumericButtonPressed(object state)
    {
        //convert state to int, update display
    }
}
public class CalculatorViewModelTest
{
    [Test]
    public void WhenNumericButtonIsPressedDigitIsShownOnDisplay()
    {
        var viewModel = new CalculatorViewModel();

        NumericButtonPressedCommand.Execute("4");

        Assert.That(viewModel.Display, Is.EqualTo("4"));

        NumericButtonPressedCommand.Execute("4");
        NumericButtonPressedCommand.Execute("2");
        NumericButtonPressedCommand.Execute("4");

        Assert.That(viewModel.Display, Is.EqualTo("4,242"));
    }
}

Context

StackExchange Code Review Q#108755, answer score: 11

Revisions (0)

No revisions yet.