WinForm Calculator

Question

I'm looking to get this cleaned up.

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

namespace BaseHexWinCalc
{
    public partial class MainCalcForm : Form
    {
        enum MathFunction
        {
            Plus,
            Minus,
            Multiply,
            Divide
        }

        public void Splash_Start()
        {
            Application.Run(new _Splash_Form());
        }
        public MainCalcForm(String name)
        {
            Thread loadSplash = new Thread(new ThreadStart(Splash_Start));
            loadSplash.Start();
            Thread.Sleep(5000);

            InitializeComponent();
            this.Text = name;

            loadSplash.Abort();
        }


        #region Operator Events
        private void Button_Ops_Click(object sender, EventArgs e)
        {
            try
            {
                MathFunction _MathFunction_Desired;
                Button selected = (Button)sender;
                String OpsVal = selected.Text;
                String text = _TextBox_Output.Text;
                Double _Double_FirstInput = Double.Parse(text);
                _Label_Current.Text += (text + OpsVal);
                switch (OpsVal)
                {
                    case "+":
                        _MathFunction_Desired = MathFunction.Plus;
                        break;
                    case "-":
                        _MathFunction_Desired = MathFunction.Minus;
                        break;
                    case "*":
                        _MathFunction_Desired = MathFunction.Multiply;
                        break;
                    case "/":
                        _MathFunction_Desired = MathFunction.Divide;
                        break;
                    default:
                        break;
                }
                _TextBox_Output.Clear();
            }
            catch { MessageBox.Show("     You must enter a value first."); }
        }
        #endregion

        #region Equals Function
        private void Button_Equals_Click(object sender, EventArgs e)
        {
            DataTable solutionFinder = new DataTable();
            String finText = _Label_Current.Text + _TextBox_Output.Text;

            if (finText.Contains("/0"))
                _TextBox_Final.Text = "No division by Zero";
            else
                try
                {
                    double finalSolution = Convert.ToDouble(solutionFinder.Compute(finText, ""));
                    _TextBox_Final.Text = (finalSolution.ToString());
                }
                catch { MessageBox.Show("     You must enter value first."); }

            _TextBox_Output.Clear(); 
            _Label_Current.Text = String.Empty;
        }
        #endregion

        #region Number Entries
        private void Button_Numeral_Click(object sender, EventArgs e)
        {            
            try
            {
                Button selected = (Button)sender;
                String NumVal = selected.Text;
                switch (NumVal)
                {
                    case "1":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "2":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "3":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "4":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "5":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "6":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "7":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "8":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "9":
                        _TextBox_Output.Text += NumVal;
                        break;
                    case "0":
                        _TextBox_Output.Text += NumVal;
                        break;
                    default:
                        break;
                }
            }
            catch { MessageBox.Show("Not a valid entry"); }
        }
        #endregion

        #region Hex Entries
        private void Button_Hex_Click(object sender, EventArgs e)
        {
            try
            {
                Button selected = (Button)sender;
                String HexVal = selected.Text;
                switch (HexVal)
                {
                    case "A":
                        _TextBox_Output.Text += HexVal;
                        break;
                    case "B":
                        _TextBox_Output.Text += HexVal;
                        break;
                    case "C":
                        _TextBox_Output.Text += HexVal;
                        break;
                    case "D":
                        _TextBox_Output.Text += HexVal;
                        break;
                    case "E":
                        _TextBox_Output.Text += HexVal;
                        break;
                    case "F":
                        _TextBox_Output.Text += HexVal;
                        break;
                    default:
                        break;
                }
            }
            catch { MessageBox.Show("Not a valid entry"); }
        }
        #endregion

        #region Non-Numeric Features
        private void Button_Dec_Click(object sender, EventArgs e)
        {
            _TextBox_Output.Text += ".";
        }
        private void _Button_Clr_Click(object sender, EventArgs e)
        {
            _TextBox_Output.Clear();
        }
        #endregion

        #region Converter
        private void Button_MainConvert_Click(object sender, EventArgs e)
        {
            if (_TextBox_Output.Text == String.Empty)
                _TextBox_Output.Text = "0";
            Button selected = (Button)sender;
            bool conCase = selected.Text.Contains("To Hex");

            if (conCase)
            {
                int numConvert;
                String finHex;
                if (Int32.TryParse(_TextBox_Output.Text, out numConvert)) 
                {
                    finHex = String.Format("{0:x}", numConvert);//create hex
                    _TextBox_Output.Clear();
                    _TextBox_Final.Text = finHex.ToUpper();//hex output
                    selected.Text = "To Dec!";              
                    HexLayout.Visible = true;
                    OperatorsLayout.Visible =
                   _Button_Equals.Visible =
                   _Button_Dec.Enabled = false;
                } else {
                    _TextBox_Final.Text = "Invalid";
                    _TextBox_Output.Clear();
                }
            }
            else
            {
                int newDec = Convert.ToInt32(_TextBox_Output.Text, 16);
                _TextBox_Output.Text = String.Empty;
                _TextBox_Final.Text = newDec.ToString();
                selected.Text = "To Hex!";
                HexLayout.Visible = false;
                OperatorsLayout.Visible =
               _Button_Equals.Visible =
               _Button_Dec.Enabled = true;
            }
        }
        #endregion

    }
}

Answer 1

If you're going to put a try catch in an else, you should probably wrap it in braces:

else 
{
    try 
    {
        ...
    }
    catch { ... }
}

You don't need to check for case "1" to case "9" and do the same thing in every single one. C# has fallthrough:

case "1":
case "2":
case "3":
    //handle cases 1 to 3
    _TextBox_Output.Text += NumVal;
    break;

And, as Dalija noted, you really don't need the switch case statement, because all of the cases do the same thing. Just do what you want to (i.e. add the the value to the output textbox.

Instead of relying on the text of the buttons, I'd rather rely on a tag. It's probably easier to do this in the designer, but:

private void SetupControls()
{
    var buttons = new Button[] { ZeroButton, OneButton, TwoButton, ThreeButton, FourButton, FiveButton, SixButton, SevenButton, EightButton, NineButton };
    for (int i = 0; i < buttons.Length; i++)
    {
        buttonValueMappings.Add(buttons[i], i.ToString());
    }
}

//later
var selected = (Button)sender;
_TextBox_Output.Text += (selected.Tag as String);

Instead of checking to see if the button contains "To Hex", keep a state variable:

bool resultIsHex = false; //default value

Flip it whenever you click the button or when a new value is computed:

resultIsHex = !resultIsHex;

Keeping track of strings gets really messy really fast. The logic becomes coupled with the UI. If you start using a different string value on the button, and you're in the habit of using strings to denote state or other things, you won't know what you just affected. If you do use string constants (don't use them for state), at least define them at the top of the file. That way if you change the string, it changes for everyone, everywhere:

public partial class Form1 : Form
{
    private const string TO_HEX = "To Hex!";
    private const string TO_DEC = "To Dec!";

    ...
}

Your Button_MainConvert_Click function has some misaligned variables. It might have been just a mistake when copying the code here:

 OperatorsLayout.Visible =
_Button_Equals.Visible =
_Button_Dec.Enabled = true;

Keep them spaced the same (or, when assigning to multiple like this at once, tab the others):

OperatorsLayout.Visible =
_Button_Equals.Visible =
_Button_Dec.Enabled = true;

//or, like I prefer:

OperatorsLayout.Visible =
    _Button_Equals.Visible =
    _Button_Dec.Enabled = true;

Putting all of the logic in events leaves you with a bunch of functionname(object sender, EventArgs e) headers. Instead, give the program a vocabulary by creating a function with a good name and calling that. For example:

private void Button_MainConvert_Click(object sender, EventArgs e)
{
    ConvertToDifferentBase();
}

private void ConvertToDifferentBase()
{
    ConvertToDifferentBases(resultIsHex); //possibly overkill
}

private void ConvertToDifferentBase(bool toHex)
{
    ...
}

You should still use int.TryParse for the hex value, because Convert.ToInt32 can throw an OverflowException:

//throws an overflow exception
var integer = Convert.ToInt32("FFFFFFFFFFFFFFFFFFFFFFFF", 16);

//this will simply return false
int integer;
bool success = int.TryParse(
    "FFFFFFFFFFFFFFFFFFFFFFFF",
    NumberStyles.HexNumber, 
    CultureInfo.CurrentCulture, 
    out integer);

//this will parse correctly and return true. integer = 250
bool success = int.TryParse(
    "FA",
    NumberStyles.HexNumber, 
    CultureInfo.CurrentCulture, 
    out integer);

Stay consistent. You used both _TextBox_Output.Clear(); and _TextBox_Output.Text = String.Empty;. Just use the Clear method in both places.

If you use {0:X} instead of {0:x} in string.Format, it will be emitted in uppercase, so you won't need your ToUpper call.

Checking if finText contains \0 is going to cause false positives, because the string could ostensibly contain \0.23 or something similar. It's better to just let the computation function throw an error, and catch that.

private void Button_Equals_Click(object sender, EventArgs e)
{
    try
    {
        int result = Compute();
    }
    catch (DivideByZeroException ex)
    {
        //handle exception, print error message
    }
}

Instead of assigning an error message to the same textbox that contains the result of the calculations, create a new textbox named ErrorTextbox at the bottom of the calculator. Set its text to red, and toggle the visibility of the textbox as needed. If I was using a calculator and it overwrote my value with an error message, I'd want my value back :(.

Instead of a switch case on the operations, put the values in a dictionary at the top of the file and just index into the dictionary (this is another time to put the string constants in one place):

public partial class MainCalcForm : Form
{
    private Dictionary<string, MathFunction> operatorMappings = new Dictionary<string, MathFunction>();

    public MainCalcForm(String name)
    {
        InitializeComponent();
        SetupOperators();
    }

    private void SetupOperators()
    {
        operatorMappings["*"] = MathFunction.Plus;
        operatorMappings["/"] = MathFunction.Minus;
        operatorMappings["+"] = MathFunction.Multiply;
        operatorMappings["-"] = MathFunction.Divide;
    }

    private void Button_Ops_Click(object sender, EventArgs e)
    {
        Button selected = (Button)sender;
        var mathFunctionDesired = operatorMappings[selected.Text];
    }
}

Check the warnings when you compile and get rid of them to make your code cleaner. You should get this warning when you compile: The variable "_MathFunction_Desired" is assigned, but its value is never used.

A lot of the code in Button_MainConvert_Click is duplicated. We can remove the duplication:

private void Button_MainConvert_Click(object sender, EventArgs e)
{
    //for converting to hex
    PerformBaseChange(String.Format("{0:X}", numConvert), true);

    //for converting to decimal
    PerformBaseChange(decimalString, false);
}

private void PerformBaseChange(string newValue, bool toHex)
{
    _TextBox_Output.Clear();
    _TextBox_Final.Text = newValue;

    //I gave it a name
    ConversionButton.Text = toHex ? "To Dec!" : "To Hex!";              

    HexLayout.Visible = toHex;
    OperatorsLayout.Visible =
        _Button_Equals.Visible =
        _Button_Dec.Enabled = !toHex;
}

I wouldn't rely on a Datatable to do your computations. You can write an algorithm pretty simply to do the computation yourself. I wrote some examples as a proof of concept. I don't check for exceptional cases, and I don't allow decimals. I kept it simple, but obviously that stuff should be added and the code should probably be refactored a bit.

One is RPN, which is extremely easy to write with a stack. Another just computes in order, and the other computes with order of operations (it's not clear to me whether you want order of operations or not).

Note that I put the computation in its own class instead of the form to keep the logic separate from the UI.

//usage
//returns 4
int result = Calculator.Compute("5 + 2 * 2 + 14 / 7");
//returns 11
int orderOfOperationsResult = Calculator.ComputeWithOrderOfOperations("5 + 2 * 2 + 14 / 7");
//returns -8
int rpnResult = Calculator.ComputeRPN("3 6 * 2 / 4 + 3 9 2 - * -");

class Operation
{
    public static Operation<int> IntegerAdd = new Operation<int>((a, b) => a + b);
    public static Operation<int> IntegerSubtract = new Operation<int>((a, b) => a - b);
    public static Operation<int> IntegerMultiply = new Operation<int>((a, b) => a * b);
    public static Operation<int> IntegerDivide = new Operation<int>((a, b) => a / b);
}

class Operation<T>
{
    public Func<T, T, T> Apply { get; private set; }

    public Operation(Func<T, T, T> compute)
    {
        Apply = compute;
    }
}

class Calculator
{
    private static Dictionary<string, Operation<int>> operatorMappings = new Dictionary<string, Operation<int>>();

    static Calculator()
    {
        SetupOperators();
    }

    private static void SetupOperators()
    {
        operatorMappings["*"] = Operation.IntegerMultiply;
        operatorMappings["/"] = Operation.IntegerDivide;
        operatorMappings["+"] = Operation.IntegerAdd;
        operatorMappings["-"] = Operation.IntegerSubtract;
    }

    public static int Compute(string equation)
    {
        var tokens = equation.Split(' ');
        var integers = new List<int>();
        var operators = new Queue<Operation<int>>();

        bool nextIsNumber = true;
        foreach (string token in tokens)
        {
            if (nextIsNumber)
            {
                integers.Add(Convert.ToInt32(token));
            }
            else
            {
                operators.Enqueue(operatorMappings[token]);
            }
            nextIsNumber = !nextIsNumber;
        }

        return integers.Aggregate((x, y) => operators.Dequeue().Apply(x, y));
    }

    public static int ComputeWithOrderOfOperations(string equation)
    {
        var tokens = equation.Split(' ');
        var integers = new List<int>();
        var operators = new List<Operation<int>>();

        var secondPassIntegers = new List<int>();
        var secondPassOperators = new Queue<Operation<int>>();

        bool nextIsNumber = true;
        foreach (string token in tokens)
        {
            if (nextIsNumber)
            {
                integers.Add(Convert.ToInt32(token));
            }
            else
            {
                operators.Add(operatorMappings[token]);
            }
            nextIsNumber = !nextIsNumber;
        }

        int currentValue = integers[0];
        for (int i = 1; i < integers.Count; i++)
        {
            var myOperator = operators[i - 1];
            //not the best way to do this.. only works because of static values
            if (myOperator == Operation.IntegerAdd || myOperator == Operation.IntegerSubtract)
            {
                secondPassIntegers.Add(currentValue);
                secondPassOperators.Enqueue(myOperator);
                currentValue = integers[i];
            }
            else
            {
                currentValue = myOperator.Apply(currentValue, integers[i]);
            }
        }
        secondPassIntegers.Add(currentValue);

        return secondPassIntegers.Aggregate((x, y) => secondPassOperators.Dequeue().Apply(x, y));
    }

    public static int ComputeRPN(string equation)
    {
        var values = equation.Split(' ');
        var stack = new Stack<int>();
        foreach (var val in values)
        {
            int num;
            if (int.TryParse(val, out num))
            {
                stack.Push(num);
            }
            else //operators
            {
                var secondNumber = stack.Pop();
                var firstNumber = stack.Pop();
                Operation<int> operation = operatorMappings[val];

                stack.Push(operation.Apply(firstNumber, secondNumber));
            }
        }

        return stack.Pop();
    }
}

Answer 2

First problem you have is that you have mixed your code logic with UI. But tackling that issue could be too much at this point. I would recommend some reading about MVC/MVP patterns and you can start with What are MVP and MVC and what is the difference?

Another visible issue is repetition in your event handlers Button_Numeral_Click and Button_Hex_Click

I presume that each of that handlers is tied with specific buttons in your UI and only numeral buttons can trigger Button_Numeral_Click just like only hex buttons can trigger Button_Hex_Click That code can be simplified and it doesn't need a switch at all.

 #region Number Entries
        private void Button_Numeral_Click(object sender, EventArgs e)
        {            
            try
            {
                Button selected = (Button)sender;
                String NumVal = selected.Text;
                _TextBox_Output.Text += NumVal;
            }
            catch { MessageBox.Show("Not a valid entry"); }
        }
        #endregion

        #region Hex Entries
        private void Button_Hex_Click(object sender, EventArgs e)
        {
            try
            {
                Button selected = (Button)sender;
                String HexVal = selected.Text;
                _TextBox_Output.Text += HexVal;
            }
            catch { MessageBox.Show("Not a valid entry"); }
        }
        #endregion

Next step in simplifying above code would be removing try..catch block, since there are no errors you would actually be catching there. And one step further would be merging Button_Numeral_Click and Button_Hex_Click into same event handler, since their logic is identical

        private void Button_Num_Hex_Click(object sender, EventArgs e)
        {
             Button selected = (Button)sender;
             String NumHexVal = selected.Text;
             _TextBox_Output.Text += NumHexVal;
        }