4
\$\begingroup\$

I tried my best to replicate, in style and functionality, the Windows standard calculator:

I would like to get some feedback on the efficiency and readability of my code as well as the approach i chose (for example the one of using the evaluate function on the expression textbox):

Form1.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;


namespace _2022_01_27
{
    public partial class Form1 : Form
    {
        bool equals_last_pressed;
        double last_number_entry = 0;
        public Form1()
        {
            InitializeComponent();
        }

        private void Add_to_entry(string digit)
        {
            if (tbox_entry.Text.Length <= 18)
            {
                tbox_entry.Text += digit;
                tbox_entry.Text = Convert.ToString(double.Parse(tbox_entry.Text));
            }
            equals_last_pressed = false;
        }

        static Double Evaluate(String expression)
        {
            System.Data.DataTable table = new System.Data.DataTable();
            return Convert.ToDouble(table.Compute(expression, String.Empty));
        }

        private void btn_0_Click(object sender, EventArgs e)
        {
            Add_to_entry("0");
        }

        private void btn_1_Click(object sender, EventArgs e)
        {
            Add_to_entry("1");
        }

        private void btn_2_Click(object sender, EventArgs e)
        {
            Add_to_entry("2");
        }

        private void btn_3_Click(object sender, EventArgs e)
        {
            Add_to_entry("3");
        }

        private void btn_4_Click(object sender, EventArgs e)
        {
            Add_to_entry("4");
        }

        private void btn_5_Click(object sender, EventArgs e)
        {
            Add_to_entry("5");
        }

        private void btn_6_Click(object sender, EventArgs e)
        {
            Add_to_entry("6");
        }

        private void btn_7_Click(object sender, EventArgs e)
        {
            Add_to_entry("7");
        }

        private void btn_8_Click(object sender, EventArgs e)
        {
            Add_to_entry("8");
        }

        private void btn_9_Click(object sender, EventArgs e)
        {
            Add_to_entry("9");
        }

        private void btn_c_Click(object sender, EventArgs e)
        {
            tbox_entry.Text = "0";
            tbox_expression.Text = "";
            last_number_entry = 0;
        }

        private void btn_ce_Click(object sender, EventArgs e)
        {
            tbox_entry.Text = "0";
        }

        private void btn_backspace_Click(object sender, EventArgs e)
        {
            tbox_entry.Text = tbox_entry.Text.Remove(tbox_entry.Text.Length - 1);
            if (tbox_entry.Text.Length == 0)
                tbox_entry.Text = "0";
        }

        private void btn_addition_Click(object sender, EventArgs e)
        {
            last_number_entry = double.Parse(tbox_entry.Text);
            if (equals_last_pressed == false)
                tbox_expression.Text += tbox_entry.Text;
            tbox_expression.Text = $"{Evaluate(tbox_expression.Text)} + ";
            tbox_entry.Text = "0";
            equals_last_pressed = false;
        }

        private void btn_subtraction_Click(object sender, EventArgs e)
        {
            last_number_entry = double.Parse(tbox_entry.Text);
            if (equals_last_pressed == false)
                tbox_expression.Text += tbox_entry.Text;
            tbox_expression.Text = $"{Evaluate(tbox_expression.Text)} - ";
            tbox_entry.Text = "0";
            equals_last_pressed = false;
        }

        private void btn_multiplication_Click(object sender, EventArgs e)
        {
            last_number_entry = double.Parse(tbox_entry.Text);
            if (equals_last_pressed == false)
                tbox_expression.Text += tbox_entry.Text;
            tbox_expression.Text = $"{Evaluate(tbox_expression.Text)} * ";
            tbox_entry.Text = "0";
            equals_last_pressed = false;
        }

        private void btn_division_Click(object sender, EventArgs e)
        {
            last_number_entry = double.Parse(tbox_entry.Text);
            if (equals_last_pressed == false)
                tbox_expression.Text += tbox_entry.Text;
            tbox_expression.Text = $"{Evaluate(tbox_expression.Text)} / ";
            tbox_entry.Text = "0";
            equals_last_pressed = false;
        }

        private void btn_sqrtx_Click(object sender, EventArgs e)
        {
            tbox_entry.Text = $"{Math.Sqrt(double.Parse(tbox_entry.Text))}";
        }

        private void btn_x2_Click(object sender, EventArgs e)
        {
            tbox_entry.Text = $"{Math.Pow(double.Parse(tbox_entry.Text), 2)}";
        }

        private void btn_1dividex_Click(object sender, EventArgs e)
        {
            if (tbox_entry.Text == "0")
                return;
            tbox_entry.Text = $"{1 / double.Parse(tbox_entry.Text)}";
        }

        private void btn_equals_Click(object sender, EventArgs e)
        {
            last_number_entry = double.Parse(tbox_entry.Text);
            if (equals_last_pressed == true)
                return;
            equals_last_pressed = true;
            tbox_expression.Text += $"{tbox_entry.Text}";
            tbox_entry.Text = Convert.ToString(Evaluate(tbox_expression.Text));
        }

        private void btn_period_Click(object sender, EventArgs e)
        {
            foreach(char digit in tbox_entry.Text)
                if (digit == '.')
                    return;
            tbox_entry.Text += ".";
        }

        private void btn_changesign_Click(object sender, EventArgs e)
        {
            if (tbox_entry.Text.Contains("-"))
                tbox_entry.Text = tbox_entry.Text.Remove(0,1);
            else
                tbox_entry.Text = "-" + tbox_entry.Text;
        }

        private void btn_percentage_Click(object sender, EventArgs e)
        {
            tbox_entry.Text = Convert.ToString(last_number_entry / 100 * double.Parse(tbox_entry.Text));
        }
    }
}
\$\endgroup\$
16
  • \$\begingroup\$ A tip: you can merge 10 handlers for digits into 1. private void btn_digit_Click(object sender, EventArgs e) { Add_to_entry((sender as Button).Text); } and assign this handler to all 10 digit buttons. Other repetitive code can be eliminated in the same way. \$\endgroup\$
    – aepot
    Commented Jan 27, 2022 at 23:19
  • 1
    \$\begingroup\$ @aepot thank you! that basically cut it in half \$\endgroup\$
    – Tommaso
    Commented Jan 28, 2022 at 7:21
  • \$\begingroup\$ One more UI tip in a project. Note that window is sizable, check the behavior. UI only, no button handlers attached but you can improve it. \$\endgroup\$
    – aepot
    Commented Jan 28, 2022 at 11:25
  • 1
    \$\begingroup\$ @PeterCsala Should've guessed there was a way to make it do that. thanks! \$\endgroup\$
    – Tommaso
    Commented Jan 28, 2022 at 15:04
  • 1
    \$\begingroup\$ @PeterCsala oh sorry, your answer definitely opened my mind to some things that i didn't even know you could do \$\endgroup\$
    – Tommaso
    Commented Feb 5, 2022 at 18:52

1 Answer 1

Reset to default
3
\$\begingroup\$

Since aepot has already mentioned a way how can you reduce the code of the btn_{n}_Click event handlers so, I will not address that problem here.

Add_to_entry

  • I think the second tbox_entry.Text assignment is pointless
  • This piece of code double.Parse(tbox_entry.Text) is being used in several places I suggest to introduce a helper method for that
private double GetEntryAsNumber()
    => double.Parse(tbox_entry.Text);

Evaluate

  • According to my understanding you don't need to create a new Table each and every time when you call this method
  • So, you can convert the table to a static member of the class
static System.Data.DataTable table = new System.Data.DataTable();
static double Evaluate(string expression)
    => Convert.ToDouble(table.Compute(expression, string.Empty));

btn_c_Click and btn_ce_Click

  • This code tbox_entry.Text = "0" is being used in multiple places so you can define a helper method for this
private void SetEntryToZero()
    => tbox_entry.Text = "0";

private void btn_c_Click(object sender, EventArgs e)
{
    SetEntryToZero();
    tbox_expression.Text = "";
    last_number_entry = 0;
}

private void btn_ce_Click(object sender, EventArgs e)
    => SetEntryToZero();

btn_addition_Click ... btn_division_Click

  • As I can see their code are almost identical except that part which comes after the Evaluate call, so the common part can be extracted
private void btn_addition_Click(object sender, EventArgs e)
    => PrepareForTheSecondOperand("+");

private void btn_subtraction_Click(object sender, EventArgs e)
    => PrepareForTheSecondOperand("-");

private void btn_multiplication_Click(object sender, EventArgs e)
    => PrepareForTheSecondOperand("*");

private void btn_division_Click(object sender, EventArgs e)
    => PrepareForTheSecondOperand("/");
    
private void PrepareForTheSecondOperand(string operation)
{
    last_number_entry = GetEntryAsNumber();
    if (!equals_last_pressed)
        tbox_expression.Text += tbox_entry.Text;
    tbox_expression.Text = $"{Evaluate(tbox_expression.Text)} {operation} ";
    SetEntryToZero();
    equals_last_pressed = false;
}
  • Instead of using the == false you can simply use the negation operator

btn_sqrtx_Click ... btn_1dividex_Click

  • All three methods can be implemented as a one-liner
    • In case of 1dividex you can use the conditional operator
private void btn_sqrtx_Click(object sender, EventArgs e)
    => tbox_entry.Text = $"{Math.Sqrt(GetEntryAsNumber())}";

private void btn_x2_Click(object sender, EventArgs e)
    => tbox_entry.Text = $"{Math.Pow(GetEntryAsNumber(), 2)}";
        
private void btn_1dividex_Click(object sender, EventArgs e)
    => tbox_entry.Text = tbox_entry.Text == "0" ? tbox_entry.Text : $"{1 / GetEntryAsNumber()}";

btn_equals_Click

  • Two minor things:
    • You don't need to use string interpolation when you do the assignment for tbox_expression.Text
    • You don't need to use Convert.ToString static method, you can simply call the ToString instance method on the double
private void btn_equals_Click(object sender, EventArgs e)
{
    last_number_entry = GetEntryAsNumber();
    if (equals_last_pressed)
        return;
    equals_last_pressed = !equals_last_pressed;
    tbox_expression.Text += tbox_entry.Text;
    tbox_entry.Text = Evaluate(tbox_expression.Text).ToString();
}

btn_period_Click, btn_changesign_Click and btn_percentage_Click

  • All three methods can be implemented as a one-liner
    • In case of period you can use Linq's Any to find a dot
    • In case of changesign you can use conditional operator
private void btn_period_Click(object sender, EventArgs e)
    => tbox_entry.Text += tbox_entry.Text.ToCharArray().Any(c => c == '.') ? "" : ".";

private void btn_changesign_Click(object sender, EventArgs e)
    => tbox_entry.Text = tbox_entry.Text.Contains("-") ? tbox_entry.Text.Remove(0, 1): "-" + tbox_entry.Text;
        
private void btn_percentage_Click(object sender, EventArgs e)
    => tbox_entry.Text = Convert.ToString(last_number_entry / 100 * GetEntryAsNumber());
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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