OP
C#: Exception handling(?)
gopi_vbboy
Cyborg Agent
Welcome to world of c#.
So here are few code suggestions-
1.Always have some good naming conventions.This helps
to improve code readability
Eg-chkFruits,nMangoes,etc
2.You can use String.Empty instead of "" for readability
3.I think as you want to validate four times using same
logic.A functions will make code tidy.
Alternate way as per OOPS ideology
is creating a validation class for you objects
which is pretty high level for now.
Like
4.Remove {} for single statements of if.
5.Dont hardcode strings.Put it in either config file
or constants.
6.You can also use
Int.TryParse for readability
So here are few code suggestions-
1.Always have some good naming conventions.This helps
to improve code readability
Eg-chkFruits,nMangoes,etc
2.You can use String.Empty instead of "" for readability
3.I think as you want to validate four times using same
logic.A functions will make code tidy.
Alternate way as per OOPS ideology
is creating a validation class for you objects
which is pretty high level for now.
Like
Code:
public void ValidateControlandAssign(CheckBox targetChkbox,TextBox tValue,TextBox tAssign,int rate )
{
private const string senterqty="Please enter a quantity";
private const string scheck="Please check the respective items after entering the quantity");
private int tempval;
if (targetChkbox.Checked)
{
if (tValue.Text != String.Empty)
tempval = Convert.ToInt32(tvalue);
else
MessageBox.Show(senterqty);
}
else
{
if (Convert.ToInt32(tValue.Text) > 0)
MessageBox.Show(scheck);
}
tAssign.Text= = Convert.ToString(rate * tempval);
}
private void button1_Click(object sender, EventArgs e)
{
..
//as per your code call
//observe how names of control of your code doesnt make sense except mango
ValidateControlandAssign(checkBox1,textBox1,textBox5.Text, rate_mango);
..
}4.Remove {} for single statements of if.
Code:
if (true) singlestatment;
else singlestatment;or constants.
Code:
private void button1_Click(object sender, EventArgs e)
{
if (checkBox1.Checked)
{
if (textBox1.Text != "")
{
qua1 = Convert.ToInt32(textBox1.Text);
}
else
{
MessageBox.Show("Please enter a quantity");
}
}
else
{
if (Convert.ToInt32(textBox1.Text) > 0)
{
MessageBox.Show("Please check the respective items after entering the quantity");
}
}
textBox5.Text = Convert.ToString(rate_mango * qua1);
if (checkBox2.Checked)
{
if (textBox2.Text != "")
{
qua1 = Convert.ToInt32(textBox2.Text);
}
else
{
MessageBox.Show("Please enter a quantity");
}
}
textBox6.Text = Convert.ToString(rate_pineapple * qua2);
if (checkBox3.Checked)
{
if (textBox3.Text != "")
{
qua1 = Convert.ToInt32(textBox3.Text);
}
else
{
MessageBox.Show("Please enter a quantity");
}
}
textBox7.Text = Convert.ToString(rate_apple * qua3);
if (checkBox4.Checked)
{
if (textBox4.Text != "")
{
qua1 = Convert.ToInt32(textBox4.Text);
}
else
{
MessageBox.Show("Please enter a quantity");
}
}
textBox8.Text = Convert.ToString(rate_banana * qua4);
textBox9.Text = Convert.ToString(Convert.ToInt32(textBox5.Text) + Convert.ToInt32(textBox6.Text) + Convert.ToInt32(textBox7.Text) + Convert.ToInt32(textBox8.Text));
}6.You can also use
Int.TryParse for readability
nbaztec
Master KOD3R
There is absolutely no need for checking String to be empty, the reason being there are only 2 equivalence classes - Correct Input and Incorrect Input and an empty string is the same as an incorrect numeric string.
@ds_
As I have earlier demonstrated the use of int.TryParse(), I suggest you instead use int.Parse() and handle the exception.
The most crude form of it would look like:
This way the calculation will cease once it detects one of the inputs as Invalid, I'd show you a much, much better way of handling things like these (using foreach and Controls property) but it'd be an overkill for now.
P.S. There is no need for Trim() here since TryParse()/Parse() trims it anyway.
@ds_
As I have earlier demonstrated the use of int.TryParse(), I suggest you instead use int.Parse() and handle the exception.
The most crude form of it would look like:
Code:
public void buttonOk_Click()
{
try {
this.itemPrice1.Text = int.Parse(this.itemQty1.Text) * itemRate1;
this.itemPrice2.Text = int.Parse(this.itemQty2.Text) * itemRate2;
this.itemPrice3.Text = int.Parse(this.itemQty3.Text) * itemRate3;
....
CalculateTotal();
} catch(FormatException) {
MessageBox.Show("Only numbers are allowed")
}
}This way the calculation will cease once it detects one of the inputs as Invalid, I'd show you a much, much better way of handling things like these (using foreach and Controls property) but it'd be an overkill for now.
P.S. There is no need for Trim() here since TryParse()/Parse() trims it anyway.
OP
gopi_vbboy said:
Thanks for your inputs but what's your take on nbaztec's opposition for 2nd point.
PS: Your 3rd and 5th point bounced my head
@nbaztec: Working on it buddy
Any views for this: C# int.Parse Optimization
Ok, am almost done, one query.
When I write else like
Code:
else;it gives Possibly mistaken empty statement warning. (runs fine though)
But when I write like:
Code:
else {}It says fine.
Everything fine now. Final queries:
Code of OK (submit) button:
Code:
private void button1_Click(object sender, EventArgs e)
{
if (checkBox1.Checked)
{
if (textBox1.Text == "")
{
MessageBox.Show("Please enter a quantity", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
}
else
{
qua1 = int.Parse(textBox1.Text);
}
}
else
{
if (textBox1.Text == "")
{
}
else
{
if (int.Parse(textBox1.Text) > 0)
MessageBox.Show("Please check the respective boxes after entering the values");
else { }
}
}
textBox5.Text = Convert.ToString(rate_mango * qua1);
if (checkBox2.Checked)
{
if (textBox2.Text == "")
{
MessageBox.Show("Please enter a quantity", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
}
else
{
qua2 = int.Parse(textBox2.Text);
}
}
else
{
if (textBox2.Text == "")
{
}
else
{
if (int.Parse(textBox2.Text) > 0)
MessageBox.Show("Please check the respective boxes after entering the values");
else { }
}
}
textBox6.Text = Convert.ToString(rate_pineapple * qua2);
if (checkBox3.Checked)
{
if (textBox3.Text == "")
{
MessageBox.Show("Please enter a quantity", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
}
else
{
qua3 = int.Parse(textBox3.Text);
}
}
else
{
if (textBox3.Text == "")
{
}
else
{
if (int.Parse(textBox3.Text) > 0)
MessageBox.Show("Please check the respective boxes after entering the values");
else { }
}
}
textBox7.Text = Convert.ToString(rate_apple * qua3);
if (checkBox4.Checked)
{
if (textBox4.Text == "")
{
MessageBox.Show("Please enter a quantity", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
}
else
{
qua4 = int.Parse(textBox4.Text);
}
}
else
{
if (textBox4.Text == "")
{
}
else
{
if (int.Parse(textBox4.Text) > 0)
MessageBox.Show("Please check the respective boxes after entering the values");
else { }
}
}
textBox8.Text = Convert.ToString(rate_banana * qua4);
textBox9.Text = Convert.ToString(int.Parse(textBox5.Text) + int.Parse(textBox6.Text) + int.Parse(textBox7.Text) + int.Parse(textBox8.Text));
}Code of "Reset" button:
Code:
private void button2_Click(object sender, EventArgs e)
{
checkBox1.Checked = false;
checkBox2.Checked = false;
checkBox3.Checked = false;
checkBox4.Checked = false;
textBox1.Text = "0";
textBox2.Text = "0";
textBox3.Text = "0";
textBox4.Text = "0";
textBox5.Text = "0";
textBox6.Text = "0";
textBox7.Text = "0";
textBox8.Text = "0";
textBox9.Text = "0";
int qua1 = 0;
int qua2 = 0;
int qua3 = 0;
int qua4 = 0;
}1) The warning messages are coming as many times as error is occurred. Like if the user enters the value for all 4 items but forgets to check the respective boxes, it gives error 4 times. How do I make it once?
2) I do a sample calculation for the first time, then press "Reset" button. Then I press "OK" button without entering any values. This should give grand total = 0 but its giving last grand total. I'm not able to catch what's wrong. It was not happening earlier, but in the modification process something went wrong.
This screenie of my app may help you overcoming my bad naming convention.
*imgur.com/sowmu.png
One more question:
I applied E.Handled restriction only to textBox1 but its working on all quantity textboxes (1-4). I can see it activated in properties, KeyPress event of all quantity textBoxes? How is it possible?
nbaztec
Master KOD3R
It's optimization at the cost of error checking. It won't throw errors on invalid inputs like "12s2". It uses the age-old technique of converting ASCII numbers to decimal.
This is compiler check. It does it since programmers sometimes tend to misplace ';' like:When I write else like
Code:else;
it gives Possibly mistaken empty statement warning. (runs fine though)
But when I write like:
Code:else {}
It says fine.
Code:
if(condition); // Erroneous semi-colon
doSomeWork();Try the try-catch approach. If you don't want to do that, then use a boolean flag to set it as true if some error occured. Then display the error only once.
I'm guessing some errand variables you failed to reset.
Possible if you copied-pasted those text-boxes from the first one.
OP
Do I really need that fuss? I just included the braces.
What is try-catch approach ? I didn't got it at all.
There are total 8 variables. 4 for inputting quantity & 4 for rate. I already gave "rest" code in above post in which I reset all the textboxes and quantity variables. Rate is of course not required.
No, I didn't.
PS: thanks for all the help
Zangetsu
I am the master of my Fate.
to catch any errors if it occurs & handle those in structured way....
catching error & displaying the user about the error
nbaztec
Master KOD3R
You can remove the else clause altogether, it's optional.
Code:
public void buttonOk_Click()
{
try {
this.itemPrice1.Text = int.Parse(this.itemQty1.Text) * itemRate1;
this.itemPrice2.Text = int.Parse(this.itemQty2.Text) * itemRate2;
this.itemPrice3.Text = int.Parse(this.itemQty3.Text) * itemRate3;
....
CalculateTotal();
} catch(FormatException) {
MessageBox.Show("Only numbers are allowed")
}
}The code seems fine, but surely there's something incorrect in the code itself.
Or you selected all the boxes, but that's irrelevant. Just select all the textboxes and remove the definition, then put it for the correct textbox.
OP
Actually I wanted to work it on all boxes and its working without my effort lol
I experienced same case in a program written by one of my friend where he made a sample calci to input two nos and print +,-,*,/ on clicking buttons. But when I saw the code, there was only "sum", nothing else.