Hopefully, you figured out yesterday’s homework and got the new function setup. In case you didn’t we will start with a refresher of the code in the next section. There you can see the solution where the issue was. It was in the signs click events. With that out of the way, we can address the next issue we have. We have very long function names because our functions do lots of things. This not only makes big long names but makes the function a Swiss Army knife of functions. We need to break our functions up so they can do only one thing.
In an ideal world, you want your function to do one thing and one thing only. Every time you need to do that thing, you have a function ready to go.
Here is are program with its Swiss Army Function
using System; using System.Windows.Forms; namespace NoobToPro { public partial class FormCalc : Form { string value1 = ""; string value2 = "" ; string results = "" ; string sign = ""; bool isValue2 = false; public FormCalc() { InitializeComponent(); } private void setValueAndUpdateDisplay(string number) { if (!isValue2) { value1 += number; results = ""; txtInput.Text = value1; } else { value2 += number; txtInput.Text = value2; } } private void setFunctionSwitchValuesAndClearInterface(string selectedSign) { txtInput.Text = ""; isValue2 = true; sign = selectedSign; } private void btn7_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("7"); } private void btn8_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("8"); } private void btn9_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("9"); } private void btn4_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("4"); } private void btn5_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("5"); } private void btn6_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("6"); } private void btn1_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("1"); } private void btn2_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("2"); } private void btn3_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("3"); } private void btn0_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("0"); } private void btnPeriod_Click(object sender, EventArgs e) { setValueAndUpdateDisplay("."); } private void btnDivide_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("/"); } private void btnMultiply_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("*"); } private void btnSubtract_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("-"); } private void btnAdd_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("+"); } private void btnEquals_Click(object sender, EventArgs e) { switch (sign) { case "+": results = (Decimal.Parse(value1) + Decimal.Parse(value2)).ToString(); break; case "-": results = (Decimal.Parse(value1) - Decimal.Parse(value2)).ToString(); break; case "*": results = (Decimal.Parse(value1) * Decimal.Parse(value2)).ToString(); break; case "/": results = (Decimal.Parse(value1) / Decimal.Parse(value2)).ToString(); break; } value1 = ""; value2 = ""; txtInput.Text = value1; isValue2 = false; lblResult.Text = results; } private void FormCalc_Load(object sender, EventArgs e) { lblResult.Text = ""; } } }
In all truth for a program this small, I would not go through the hassle of applying this principle here. It would make the code more difficult to maintain and give you no advantages elsewhere. That being said, that is technical not good coding and could really hurt you on bigger projects so we are going to break it down a bit more.
If we look at the setValueAndUpdateDisplay function we have three things that are going on in this function.
- Determining which value to process
- Resetting the Results if value 1
- Setting the txtInput control to a chosen value
One of these is clearly a GUI item. Lets separate it out of the function and put it back into the buttons. Now in order to do that we need to refactor setValueAndUpdateDisplay function. We are after all removing the Update Display part. The new function will need to understand what to set the text of the textbox to so lets return a string.
private void setValueAndUpdateDisplay(string number) { //determining which value to process //resets results if value 1 //sets the txtInput control to chosen value --GUI if (!isValue2) { value1 += number; results = ""; txtInput.Text = value1; } else { value2 += number; txtInput.Text = value2; } }Before
private string setValue(string number) { if (!isValue2) { value1 += number; results = ""; return value1; } value2 += number; return value2; }After
A good question might be why I left the resetting of the results here? The answer is the results resetting is directly tied to the value1 being reset. Because that logic will likely never change since it is a characteristic of value 1 being set. I feel like it is ok to leave in here. That being said you could take this as far down as you like.
For the eagle eyed reader, you noticed the else disappeared. Because the return value breaks out of the function we don’t need the else here. It is just wasted code. I trimmed it off as I try to get the code to be tighter.
What about the buttons.
Because we are now returning the value we set, we can have the buttons, which are part of the GUI, deal with GUI things. I prefer to have my GUI modify the state of my GUI. I generally dislike having a function try to deal with it. It prevents the functions from being portable.
Speaking of portable, these are not there yet. Because we are depending on global variables these functions are stuck in place. They are what is known as tightly coupled.
If you are feeling bold then give this process a try with the setFunctionSwitchValuesAndClearInterface function.
Here is the code.
using System; using System.Windows.Forms; namespace NoobToPro { public partial class FormCalc : Form { string value1 = ""; string value2 = "" ; string results = "" ; string sign = ""; bool isValue2 = false; public FormCalc() { InitializeComponent(); } private string setValue(string number) { if (!isValue2) { value1 += number; results = ""; return value1; } value2 += number; return value2; } private void setFunctionSwitchValuesAndClearInterface(string selectedSign) { txtInput.Text = ""; isValue2 = true; sign = selectedSign; } private void btn7_Click(object sender, EventArgs e) { txtInput.Text = setValue("7"); } private void btn8_Click(object sender, EventArgs e) { txtInput.Text = setValue("8"); } private void btn9_Click(object sender, EventArgs e) { txtInput.Text = setValue("9"); } private void btn4_Click(object sender, EventArgs e) { txtInput.Text = setValue("4"); } private void btn5_Click(object sender, EventArgs e) { txtInput.Text = setValue("5"); } private void btn6_Click(object sender, EventArgs e) { txtInput.Text = setValue("6"); } private void btn1_Click(object sender, EventArgs e) { txtInput.Text = setValue("1"); } private void btn2_Click(object sender, EventArgs e) { txtInput.Text = setValue("2"); } private void btn3_Click(object sender, EventArgs e) { txtInput.Text = setValue("3"); } private void btn0_Click(object sender, EventArgs e) { txtInput.Text = setValue("0"); } private void btnPeriod_Click(object sender, EventArgs e) { txtInput.Text = setValue("."); } private void btnDivide_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("/"); } private void btnMultiply_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("*"); } private void btnSubtract_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("-"); } private void btnAdd_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("+"); } private void btnEquals_Click(object sender, EventArgs e) { switch (sign) { case "+": results = (Decimal.Parse(value1) + Decimal.Parse(value2)).ToString(); break; case "-": results = (Decimal.Parse(value1) - Decimal.Parse(value2)).ToString(); break; case "*": results = (Decimal.Parse(value1) * Decimal.Parse(value2)).ToString(); break; case "/": results = (Decimal.Parse(value1) / Decimal.Parse(value2)).ToString(); break; } value1 = ""; value2 = ""; txtInput.Text = value1; isValue2 = false; lblResult.Text = results; } private void FormCalc_Load(object sender, EventArgs e) { lblResult.Text = ""; } } }
Remember my general rule that GUI things change GUIs. This one is likely counterproductive but I promise it is setting something big up.
Speaking of good ideas, all these global variables is starting to cramp our style. Before we go, did anyone else catch the useless piece of code? I will give you a hint. It is a slight pain in our bums.
The joys of refactoring is that sometimes you get bonus code. When you find it, it needs to be eliminated. I will comment on it in the next section.
Finally, at this point you are probably writing annoying but starting to get manageable code. A whole generation of procedural coders built apps that look like this one. This is not good but it is the reality.
Here is the solution
using System; using System.Windows.Forms; namespace NoobToPro { public partial class FormCalc : Form { string value1 = ""; string value2 = "" ; string results = "" ; string sign = ""; bool isValue2 = false; public FormCalc() { InitializeComponent(); } private string setValue(string number) { if (!isValue2) { value1 += number; results = ""; return value1; } value2 += number; return value2; } private void setFunctionSwitchValuesAndClearInterface(string selectedSign) { sign = selectedSign; isValue2 = true; } private void btn7_Click(object sender, EventArgs e) { txtInput.Text = setValue("7"); } private void btn8_Click(object sender, EventArgs e) { txtInput.Text = setValue("8"); } private void btn9_Click(object sender, EventArgs e) { txtInput.Text = setValue("9"); } private void btn4_Click(object sender, EventArgs e) { txtInput.Text = setValue("4"); } private void btn5_Click(object sender, EventArgs e) { txtInput.Text = setValue("5"); } private void btn6_Click(object sender, EventArgs e) { txtInput.Text = setValue("6"); } private void btn1_Click(object sender, EventArgs e) { txtInput.Text = setValue("1"); } private void btn2_Click(object sender, EventArgs e) { txtInput.Text = setValue("2"); } private void btn3_Click(object sender, EventArgs e) { txtInput.Text = setValue("3"); } private void btn0_Click(object sender, EventArgs e) { txtInput.Text = setValue("0"); } private void btnPeriod_Click(object sender, EventArgs e) { txtInput.Text = setValue("."); } private void btnDivide_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("/"); txtInput.Text = ""; } private void btnMultiply_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("*"); txtInput.Text = ""; } private void btnSubtract_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("-"); txtInput.Text = ""; } private void btnAdd_Click(object sender, EventArgs e) { setFunctionSwitchValuesAndClearInterface("+"); txtInput.Text = ""; } private void btnEquals_Click(object sender, EventArgs e) { switch (sign) { case "+": results = (Decimal.Parse(value1) + Decimal.Parse(value2)).ToString(); break; case "-": results = (Decimal.Parse(value1) - Decimal.Parse(value2)).ToString(); break; case "*": results = (Decimal.Parse(value1) * Decimal.Parse(value2)).ToString(); break; case "/": results = (Decimal.Parse(value1) / Decimal.Parse(value2)).ToString(); break; } value1 = ""; value2 = ""; txtInput.Text = value1; isValue2 = false; lblResult.Text = results; } private void FormCalc_Load(object sender, EventArgs e) { lblResult.Text = ""; } } }
Yep, I stuck the txtInput reset back into the button.
Before I was not worried about Swiss Army functions and potentially none GUI functions dealing with it. Now we do so it goes back.
If there were multiple things happening, I would probably wrap that into a function. Since there is only the one change to the GUI so I will avoid the extra work.
If you don’t know what to do then wrap the txtInput.text = “” into its own function and then replace that text with the function name in each button click. In theory if there ever was a reason to add more logic to the button click that affected the GUI, you have a ready place to put it.