When I was a young pup, I wrote really awful code. Most programmers when they start, write really bad code. Here are a couple of things to remember. No matter how ugly the code is, if you get a working program then you were successful. However, that success does not mean that the poor bastard that has to maintain it will thank you. That poor SOB at least wants code clarity.
As we go through this remember, get the code working to make your customer happy. Refactor to make your maintenance programmer happy. This section will focus on making what is here more readable. This will help us start to refactor the code. Think of this as step 1 in refactoring. So lets dig into some really awful code and code review my Noob 4 Function Calculator for clarity.
The 4 function calculator, Noob Style
So first lets look at the GUI.
Not great but it works. It has the 4 function of add, subtract, multiple and divide. We have all ten numbers between 0 and 9. There is even a period for floating point.
Lets dig into the code.
using System; using System.Windows.Forms; namespace NoobToPro { public partial class FormCalc : Form { string value1 = ""; string value2 = "" ; string results = "" ; string s = ""; bool iv2 = false; public FormCalc() { InitializeComponent(); } private void btn7_Click(object sender, EventArgs e) { if(iv2 == false) { value1 = value1 + "7"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "7"; txtInput.Text = value2; } } private void btn8_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "8"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "8"; txtInput.Text = value2; } } private void btn9_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "9"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "9"; txtInput.Text = value2; } } private void btn4_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "4"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "4"; txtInput.Text = value2; } } private void btn5_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "5"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "5"; txtInput.Text = value2; } } private void btn6_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "6"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "6"; txtInput.Text = value2; } } private void btn1_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "1"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "1"; txtInput.Text = value2; } } private void btn2_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "2"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "2"; txtInput.Text = value2; } } private void btn3_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "3"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "3"; txtInput.Text = value2; } } private void btn0_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "0"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "0"; txtInput.Text = value2; } } private void btnPeriod_Click(object sender, EventArgs e) { if (iv2 == false) { value1 = value1 + "."; results = value1; txtInput.Text = value1; } else { value2 = value2 + "."; txtInput.Text = value2; } } private void btnDivide_Click(object sender, EventArgs e) { txtInput.Text = ""; iv2 = true; s = "/"; } private void btnMultiply_Click(object sender, EventArgs e) { txtInput.Text = ""; iv2 = true; s = "*"; } private void btnSubtract_Click(object sender, EventArgs e) { txtInput.Text = ""; iv2 = true; s = "-"; } private void btnAdd_Click(object sender, EventArgs e) { txtInput.Text = ""; iv2 = true; s = "+"; } private void btnEquals_Click(object sender, EventArgs e) { if (s == "+") { results = (Decimal.Parse(value1) + Decimal.Parse(value2)).ToString(); } else if(s == "-") { results = (Decimal.Parse(value1) - Decimal.Parse(value2)).ToString(); } else if (s == "*") { results = (Decimal.Parse(value1) * Decimal.Parse(value2)).ToString(); } else if (s == "/") { results = (Decimal.Parse(value1) / Decimal.Parse(value2)).ToString(); } value1 = ""; value2 = ""; txtInput.Text = value1; iv2 = false; lblResult.Text = results; } private void FormCalc_Load(object sender, EventArgs e) { lblResult.Text = ""; } } }
Wow, were does one start with this absolute noob code?
Code Review, Refactor for Clarity
So The first thing we should probably focus on is cleaning up the naming. I was never very bad at naming. Other then naming my variables in foreach loops with and i, I don’t usual getting in trouble with myself over such things.
Lets start with s. Digging in the code, we can see in the btnEquals_Click, that it is probably the sign. Because it starts with s that is probably what was meant by earlier me. So lets rename s to sign. Value1 and Value2 are the first value in the equation and the second value. Results holds the results of the calculation so we are good there too. However we have iv2. A boolean with a cryptic name. Looking the code, we can see in each of the button click events for the numbers that it is used to assign value to 1 or 2. Looking at this iv2 is probably isValue2. Lets change that so it is clearer. Keep good code hygiene and make sure the variables, constants, object and functions are all clearly named. If you have to have a longer name then do it.
To all the people saying that is too much typing. I say. Quit using VI editor and switch to a modern IDE that autocompletes. With that concluded, I see no massive naming issues. Next lets tackle the wordiness.
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 btn7_Click(object sender, EventArgs e) { if(isValue2 == false) { value1 = value1 + "7"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "7"; txtInput.Text = value2; } } private void btn8_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "8"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "8"; txtInput.Text = value2; } } private void btn9_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "9"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "9"; txtInput.Text = value2; } } private void btn4_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "4"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "4"; txtInput.Text = value2; } } private void btn5_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "5"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "5"; txtInput.Text = value2; } } private void btn6_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "6"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "6"; txtInput.Text = value2; } } private void btn1_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "1"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "1"; txtInput.Text = value2; } } private void btn2_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "2"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "2"; txtInput.Text = value2; } } private void btn3_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "3"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "3"; txtInput.Text = value2; } } private void btn0_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "0"; results = ""; txtInput.Text = value1; } else { value2 = value2 + "0"; txtInput.Text = value2; } } private void btnPeriod_Click(object sender, EventArgs e) { if (isValue2 == false) { value1 = value1 + "."; results = value1; txtInput.Text = value1; } else { value2 = value2 + "."; txtInput.Text = value2; } } private void btnDivide_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "/"; } private void btnMultiply_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "*"; } private void btnSubtract_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "-"; } private void btnAdd_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "+"; } private void btnEquals_Click(object sender, EventArgs e) { if (sign == "+") { results = (Decimal.Parse(value1) + Decimal.Parse(value2)).ToString(); } else if(sign == "-") { results = (Decimal.Parse(value1) - Decimal.Parse(value2)).ToString(); } else if (sign == "*") { results = (Decimal.Parse(value1) * Decimal.Parse(value2)).ToString(); } else if (sign == "/") { results = (Decimal.Parse(value1) / Decimal.Parse(value2)).ToString(); } value1 = ""; value2 = ""; txtInput.Text = value1; isValue2 = false; lblResult.Text = results; } private void FormCalc_Load(object sender, EventArgs e) { lblResult.Text = ""; } } }
De-wording the C#
This is really wordy C#. Remember, I am from a Visual Basic.NET background and this is too wordy for me. Let’s clean some of that up. First lets change all the isValue2== false conditions in the number buttons. There is a cleaner less wordy way of saying thing x is false. Use the ! at the start of your expression evaluation. So swap out the “if(isValue2 == false)” to “is(!isValue2)”. That should make it look a little less blocky.
Because we have the conditionals improved, lets improve the rest of the block.
Everywhere we have a valueX = valueX + string style command lets switch it to a valueX += string. This will clean up the wordiness quite a bit. For example, if you have value1 = value1 + “1” we will cleanup that line to value1 += “1”.
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 btn7_Click(object sender, EventArgs e) { if(!isValue2) { value1 += "7"; results = ""; txtInput.Text = value1; } else { value2 += "7"; txtInput.Text = value2; } } private void btn8_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "8"; results = ""; txtInput.Text = value1; } else { value2 += "8"; txtInput.Text = value2; } } private void btn9_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "9"; results = ""; txtInput.Text = value1; } else { value2 += "9"; txtInput.Text = value2; } } private void btn4_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "4"; results = ""; txtInput.Text = value1; } else { value2 += "4"; txtInput.Text = value2; } } private void btn5_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "5"; results = ""; txtInput.Text = value1; } else { value2 += "5"; txtInput.Text = value2; } } private void btn6_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "6"; results = ""; txtInput.Text = value1; } else { value2 += "6"; txtInput.Text = value2; } } private void btn1_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "1"; results = ""; txtInput.Text = value1; } else { value2 += "1"; txtInput.Text = value2; } } private void btn2_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "2"; results = ""; txtInput.Text = value1; } else { value2 += "2"; txtInput.Text = value2; } } private void btn3_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "3"; results = ""; txtInput.Text = value1; } else { value2 += "3"; txtInput.Text = value2; } } private void btn0_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "0"; results = ""; txtInput.Text = value1; } else { value2 += "0"; txtInput.Text = value2; } } private void btnPeriod_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "."; results = value1; txtInput.Text = value1; } else { value2 += "."; txtInput.Text = value2; } } private void btnDivide_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "/"; } private void btnMultiply_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "*"; } private void btnSubtract_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "-"; } private void btnAdd_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "+"; } private void btnEquals_Click(object sender, EventArgs e) { if (sign == "+") { results = (Decimal.Parse(value1) + Decimal.Parse(value2)).ToString(); } else if(sign == "-") { results = (Decimal.Parse(value1) - Decimal.Parse(value2)).ToString(); } else if (sign == "*") { results = (Decimal.Parse(value1) * Decimal.Parse(value2)).ToString(); } else if (sign == "/") { results = (Decimal.Parse(value1) / Decimal.Parse(value2)).ToString(); } value1 = ""; value2 = ""; txtInput.Text = value1; isValue2 = false; lblResult.Text = results; } private void FormCalc_Load(object sender, EventArgs e) { lblResult.Text = ""; } } }
That is much nicer and breezier code to work with in the future. There is one more thing I think we should do in this clarity section to improve this codes readability. I think we should switch out the btnEquals_Click if for a switch statement.
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 btn7_Click(object sender, EventArgs e) { if(!isValue2) { value1 += "7"; results = ""; txtInput.Text = value1; } else { value2 += "7"; txtInput.Text = value2; } } private void btn8_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "8"; results = ""; txtInput.Text = value1; } else { value2 += "8"; txtInput.Text = value2; } } private void btn9_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "9"; results = ""; txtInput.Text = value1; } else { value2 += "9"; txtInput.Text = value2; } } private void btn4_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "4"; results = ""; txtInput.Text = value1; } else { value2 += "4"; txtInput.Text = value2; } } private void btn5_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "5"; results = ""; txtInput.Text = value1; } else { value2 += "5"; txtInput.Text = value2; } } private void btn6_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "6"; results = ""; txtInput.Text = value1; } else { value2 += "6"; txtInput.Text = value2; } } private void btn1_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "1"; results = ""; txtInput.Text = value1; } else { value2 += "1"; txtInput.Text = value2; } } private void btn2_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "2"; results = ""; txtInput.Text = value1; } else { value2 += "2"; txtInput.Text = value2; } } private void btn3_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "3"; results = ""; txtInput.Text = value1; } else { value2 += "3"; txtInput.Text = value2; } } private void btn0_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "0"; results = ""; txtInput.Text = value1; } else { value2 += "0"; txtInput.Text = value2; } } private void btnPeriod_Click(object sender, EventArgs e) { if (!isValue2) { value1 += "."; results = value1; txtInput.Text = value1; } else { value2 += "."; txtInput.Text = value2; } } private void btnDivide_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "/"; } private void btnMultiply_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "*"; } private void btnSubtract_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "-"; } private void btnAdd_Click(object sender, EventArgs e) { txtInput.Text = ""; isValue2 = true; sign = "+"; } 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 = ""; } } }
I think I will wrap this up here. Don’t get me wrong this code has a bunch of issues with it still. At least at this point, it will be easier to follow as we address the rest of those issues.
Remember to name your variables with meaningful names. Don’t get concerned about the length. If someone tells you otherwise, it is because they are old and lazy. Don’t follow their example unless you are programming in GW Basic or something.