C# WPF Simple Calculator

Question

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;
                break;
            case CalcOperator.Times:
                result = a * b;
                break;
            case CalcOperator.Divide:
                if (b == 0.0)
                    result = 0.0;
                else
                    result = (double)a / b;
                break;
        }

        txtDisp.Text = result.ToString();
        lastOper = CalcOperator.None;
        a = 0;
        b = 0;
    }

    private void Window_Loaded(object sender, RoutedEventArgs e)
    {
        btnPlus.Tag = CalcOperator.Plus;
        btnMinus.Tag = CalcOperator.Minus;
        btnTimes.Tag = CalcOperator.Times;
        btnDivide.Tag = CalcOperator.Divide;
    }

    private void OnClickDigit(object sender, RoutedEventArgs e)
    {
        Button btn = sender as Button;
        HandleDigit(Convert.ToInt16(btn.Content.ToString()));
    }

    private void OnClickOperator(object sender, RoutedEventArgs e)
    {
        Button btn = sender as Button;
        HandleOperator((CalcOperator)btn.Tag);
    }

    private void btnEqual_Click(object sender, RoutedEventArgs e)
    {
        Compute();
    }

    private void btnDot_Click(object sender, RoutedEventArgs e)
    {
        HandleDecimal();
    }

}

Answer 1

MVVM pattern

This pattern can help you separate business logic and UI in a clear and testable manner.

CalculatorState and CalculatorLogic is your model/BL and it can be made shareable. Any calculation can take your model and modify it with a result or with an error message (divide by zero etc). It represents the entire I/O of your business logic (calculation on a calculator), you could expand it to support new features, such as history, with minimum refactoring of other code.

CalculatorWindow/xaml is your UI, this is the visible part of your code

CalculatorVM is the glue between UI and model.

// model class
public class CalculatorState : INotifyPropertyChanged /*IMPLEMENT, ON ALL PROPERTIES */ {  
  public bool IsError{get;set;}
  public string ErrorMessage{get;set;} 

  // Value is what's on the calculator screen under normal conditions
  public double Value {get;set;}

  // the calculator's memory
  private double? mem;
  public double Mem {
    get { return mem.GetValueOrDefault(Value); } 
    set { mem = value; }
  }
}

// business logic
public static class CalculatorLogic{
  public static readonly Action<CalculatorState, double?> Add = (state,prm)=>state.Value = state.Mem + state.Value;
  public static readonly Action<CalculatorState, double?> Sub = (state,prm)=>state.Value = state.Mem - state.Value;
}

// VM component
public class CalculatorCommand: ICommand<double?>{
  public CalculatorState State {get;set;}

  public readonly Action<CalculatorState, double?> Calculate;
  public readonly bool IsTwoOpCommand;

  public CalculatorCommand(Action<CalculatorState, double?> calculate, CalculatorState state = null, bool isTwoOpCommand = true){
    Calculate = calculate;
    State = state;
    IsTwoOpCommand = isTwoOpCommand;
  }

  public void Execute(double? prm){
    if (State!=null){
      if (Calculate!=null){
         // for two-op commands without a Mem put the Value in Mem 
         if (!IsTwoOpCommand || State.Mem.HasValue)
           Calculate(State);
         else
           State.Mem = State.Value;
      } else {
        State.IsError = true;
        State.ErrorMessage = "Null function";
      }
    } else // throw if you wish  
      Debug.WriteLine("Unexpected empty state");
  }
}

// View-Model, links your UI to the model
public CalculatorVM : INotifyPropertyChanged {
  public readonly CalculatorState State;

  public readonly ICommand AddCommand;
  public readonly ICommand SubCommand;
....
  public readonly ICommand NumberCommand;

  public CalculatorVM(CalculatorState state){
    State = state;
    NumberCommand = new CalculatorCommand(c,p=>c.Value = c.Value*10 + p, State, false);
    SubCommand = new CalculatorCommand(CalculatorLogic.Sub State);
    AddCommand = new CalculatorCommand(CalculatorLogic.Add, State);
  }
}

// View (UI). If you did the rest of the work your UI class should be mostly empty,
// most of the setup would be done in the declarative part (XAML) via bindings
// this allows you to reuse your entire business logic, unit test included
// when you decide to switch platforms (desktop, mobile, server)
public CalculatorWindow: Window{

  public CalculatorWindow(){ 
    BindingContext = new CalculatorVM(new CalculatorState()); 
    InitializeComponent(); 
  }
}

CalculatorWindow.xaml:

<CalculatorWindow....>
...
  <TextBlock Text="{Binding State.Value}"/>
...
  <TextBlock Text="{Binding State.ErrorMessage}" Visibility="{Binding State.IsError, Converter={...bool-to-visible-converter}}"/>
...
  <Button Command="{Binding NumberCommand}" CommandParameter="0">0</Button> 
... 
  <Button Command="{Binding AddCommand}">+</Button>  
</CalculatorWindow>

Answer 2

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)

Answer 3

Here're things that I think should be improved:

---

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

Encapsulate those in a class/struct like UserInput

---

public void HandleDigit(int i)

i is not a good parameter name, it should be called digit if this is what your method handles.

---

lastOper 

I think names shouldn't usually be abbreviated if it's not a very common abbreviation. In this case I would use the full name lastOperation.

The same applies to several other variables/parameters: oper, decimSepar, txtDisp etc.

---

// handle calculator operation (button)
public void HandleOperator(CalcOperator oper)

Those comments are not very helpful because intellisense won't show them unless you make them xml-comments. It's also irrelevant what calls it so the word (button) does not help much either.

---

public void Compute() { ... }

This method should only return the result:

public double Compute() { ... }

Currently it has more than one responsibility because it also prints the result:

txtDisp.Text = result.ToString();

Move this line into a new method like UpdateTextBox and reset the lastOper also after calling the Compute method. You do the same in several other places too where you update the UI for example inside the HandleOperator.

Try to separate the calculator from the UI so that you can test it separately. Currently you wouldn't be able to write any unit test for it. Rename your window to CalculatorWindow and move the entire calculator logic into a new class called Calculator that does nothing else but calculating ;-)

This will have the advantage that if you later decide to use MVVM or ASP.NET or anything else for your calculator app you'll just need to write a new UI not the calculator itself.

Answer 4

According to t3chb0t's answer I tried to make the code from my question better. But I'm not still sure about separation of UI from Calculator logic. In CalculatorWindow, I create Calculator instance, which instance of CalculatorWindow is injected into. In CalculatorWindow class, I'm now doing nothing instead of UI modification. The buttons Tag property is set inside of XAML.

public partial class CalculatorWindow : Window
{
    /// <summary>
    /// decimal separator of current culture
    /// </summary>
    private char decimalSeparator = Convert.ToChar
        (CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator);

    private Calculator calculator;

    public string DisplayText
    {
        get { return txtDisplay.Text; }
        set { txtDisplay.Text = value; }
    }

    public CalculatorWindow()
    {
        calculator = new Calculator(this);
        InitializeComponent();
    }

    /// <summary>
    /// clicked on digit button
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    private void OnClickDigit(object sender, RoutedEventArgs e)
    {
        Button btn = sender as Button;
        calculator.HandleDigit(Convert.ToInt16(btn.Content.ToString()));
    }

    /// <summary>
    /// clicked on operation button
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    private void OnClickOperator(object sender, RoutedEventArgs e)
    {
        Button btn = sender as Button;
        calculator.SetOperation((CalcOperator)btn.Tag);
    }

    /// <summary>
    /// clicked on equal button
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    private void btnEqual_Click(object sender, RoutedEventArgs e)
    {
        DisplayText = calculator.Compute().ToString();
    }

    /// <summary>
    /// clicked on decimal button
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    private void btnDecimal_Click(object sender, RoutedEventArgs e)
    {
        if (!DisplayText.Contains(decimalSeparator))
            DisplayText += decimalSeparator;
    }
}

The next is CalcOperator enum and UserInput data implementation. Here, I see the only problem with public members of UserInput, but because it's very simple class, I've kept it to be light.

/// <summary>
/// calculator operations
/// </summary>
public enum CalcOperator
{
    None,
    Plus,
    Minus,
    Times,
    Divide
}

/// <summary>
/// user input data
/// </summary>
public class UserInput
{
    public double a = 0.0;
    public double b = 0.0;
    public CalcOperator operation = CalcOperator.None;
}

And finally implementation of Calculator logic class. Here I feel that public void HandleDigit(int digit) function isn't implemented properly, as it touches CalculatorWindow's UI TextBox.Text property. How could be this improved now?

/// <summary>
/// calculator engine
/// </summary>
class Calculator
{
    private UserInput input = new UserInput();
    private CalculatorWindow window;

    public Calculator(CalculatorWindow window)
    {
        this.window = window;
    }

    /// <summary>
    /// compute the result
    /// </summary>
    /// <returns></returns>
    public double Compute()
    {
        double result = 0.0;

        switch (input.operation)
        {
            case CalcOperator.Plus:
                result = input.a + input.b;
                break;
            case CalcOperator.Minus:
                result = input.a - input.b;
                break;
            case CalcOperator.Times:
                result = input.a * input.b;
                break;
            case CalcOperator.Divide:
                if (input.b == 0.0)
                    result = 0.0;
                else
                    result = (double)input.a / input.b;
                break;
        }

        input.operation = CalcOperator.None;
        input.a = 0;
        input.b = 0;

        return result;
    }

    /// <summary>
    /// set user input operation
    /// </summary>
    /// <param name="operation"></param>
    public void SetOperation(CalcOperator operation)
    {
        input.operation = operation;
    }

    /// <summary>
    /// handle single digit
    /// </summary>
    /// <param name="digit"></param>
    public void HandleDigit(int digit)
    {
        string strNum = window.DisplayText;
        double oldNum = Convert.ToDouble(strNum);

        if (oldNum == 0 ||
            input.operation == CalcOperator.None && input.a != oldNum ||
            input.operation != CalcOperator.None && input.b != oldNum)
        {
            strNum = string.Empty;
        }

        strNum += digit.ToString();

        double newNum = Convert.ToDouble(strNum);

        if (input.operation == CalcOperator.None)
            input.a = newNum;
        else
            input.b = newNum;

        window.DisplayText = strNum;
    }
}