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

WinForm Calculator

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

Problem

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;

Solution

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 operatorMappings = new Dictionary();

public MainCalcForm(String name)
{
InitializeCompone

Code Snippets

else 
{
    try 
    {
        ...
    }
    catch { ... }
}
case "1":
case "2":
case "3":
    //handle cases 1 to 3
    _TextBox_Output.Text += NumVal;
    break;
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);
bool resultIsHex = false; //default value
resultIsHex = !resultIsHex;

Context

StackExchange Code Review Q#87273, answer score: 7

Revisions (0)

No revisions yet.