9
\$\begingroup\$

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();
    }

}
\$\endgroup\$
0

4 Answers 4

4
\$\begingroup\$

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>
\$\endgroup\$
3
  • \$\begingroup\$ So this is MVVM pattern used here? I can see the basic idea, but many new things for me in your answer. At least it shows me what to focus on ;) Thanks! \$\endgroup\$
    – Majak
    Oct 26, 2015 at 19:29
  • \$\begingroup\$ @Majak yes. This is the basic outline of MVVM. It's not 100% accurate, but close enough to give you a good start \$\endgroup\$ Oct 26, 2015 at 20:13
  • \$\begingroup\$ @Majak yes, it's obviously just a sketch, but it should illustrate how to lay out your code so it's easy enough to start with adding two numbers and still easy enough to build a full scientific calculator, unit-testable and reusable across platforms. \$\endgroup\$ Oct 26, 2015 at 23:55
11
\$\begingroup\$

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)

\$\endgroup\$
2
  • \$\begingroup\$ I really like your unit test example for the calculator! \$\endgroup\$
    – Majak
    Oct 26, 2015 at 13:40
  • 1
    \$\begingroup\$ I also think it it the way to go but the OP first needs to separate the calculator from the UI so he can write test at all ;-) I didn't want to suggest MVVM at this point yet. But if the code is refactored I think it is much easier then to build anything upon it. +1 \$\endgroup\$
    – t3chb0t
    Oct 26, 2015 at 15:04
8
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ About XML comments, I still can't get familiar with them. I know that it's official recommended commenting style and there isn't any intellisense without them, but code looks so ugly and bloated for me. But I know, it's my personal problem. \$\endgroup\$
    – Majak
    Oct 26, 2015 at 10:49
  • \$\begingroup\$ I agree. I'm also not a big fan of them. However if you write code that others will use it is unavoidable because it's the only way to describe what a method does or what are parameters for. If I write code only for me I almost never write xml comments I probably shouldn't have said it ;-) \$\endgroup\$
    – t3chb0t
    Oct 26, 2015 at 10:55
  • \$\begingroup\$ I would use them in some library or in whatever code which I want to publish. But yes I agree with you, here I'm also publishing piece of code, so proper comments should be here. \$\endgroup\$
    – Majak
    Oct 26, 2015 at 11:02
  • \$\begingroup\$ I've added one last sentence to my review why it is a good idea to separate the calculator from the UI. Now I see you already plan to do it ;-) \$\endgroup\$
    – t3chb0t
    Oct 26, 2015 at 11:04
  • 1
    \$\begingroup\$ I took the liberty to answer my question myself and show refactored code. If you are so kind to lost a few more minutes for me and look on it, I'll be very glad. \$\endgroup\$
    – Majak
    Oct 26, 2015 at 14:59
0
\$\begingroup\$

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;
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ I fail to see the point of these documentation comments - "clicked on decimal button", "user input data" etc. all of this goes without saying. Comments should serve to explain something, if they're only reiterating the obvious they're a waste of space. It's not a goal in and of itself to have as many comments as possible ;) \$\endgroup\$ Oct 26, 2015 at 15:02
  • \$\begingroup\$ @Konrad Morawski: I agree with you. The next time I will choose better what to comment. \$\endgroup\$
    – Majak
    Oct 26, 2015 at 15:45
  • \$\begingroup\$ It's much better now ;-) however the HandleDigit method bothers me because it's still not completely clean with its windows.DisplayText. \$\endgroup\$
    – t3chb0t
    Oct 26, 2015 at 16:29
  • \$\begingroup\$ Yes I feel the same. But how to avoid this effectively? \$\endgroup\$
    – Majak
    Oct 26, 2015 at 19:00
  • 1
    \$\begingroup\$ @t3chb0t and OP, please don't do reviews inside review answers. A better way would be to ask a follow up question. \$\endgroup\$
    – Heslacher
    Oct 27, 2015 at 5:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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