button click event C#

Question

So I have a Windows form with a few fields to accept data and write to a SQL database. My button_click event is what handles all the work except for validating the user input. These are handling by separate methods. My question is that if a user has incorrect input, how do I reset the form after showing the messagebox? Maybe just use the button click event to start a separate method so I can control it better?

private void enter_button_Click(object sender, EventArgs e)
{
    restart:
    try {
        try {
                Validate(fname);
                Validate(lname);
                Validate(city);
                Validate(state);
        } catch (Exception ex) {
            MessageBox.Show(ex.Message);
            if (ex != null) {
                fname.Clear();
                lname.Clear();
                city.Clear();
                state.Clear();
                goto restart;
            }
        }
        try {
            exValidate(address);
        } catch (Exception ex1) {
            MessageBox.Show(ex1.Message);
            if (ex1 != null) {
                address.Clear();
                goto restart;
            }
        }
        //blah blah...write to database.
    }
    // ...
}

Why would you want to "restart" (revalidate) right after you've just cleared all the textboxes? By the way, using labels in C# like that is not standard and very unreadable. — SimpleVar, Jun 08 '12 at 14:54
Take a look at [something](https://files.ifi.uzh.ch/rerg/arvo/courses/kvse/uebungen/Dijkstra_Goto.pdf) which is part of the history of computer science :-) — Francesco Baruchelli, Jun 08 '12 at 14:57
http://stackoverflow.com/questions/46586/goto-still-considered-harmful — Arran, Jun 08 '12 at 14:57
Also, you have too much try catching going on. You have a serious design flaw if all your methods are throwing exceptions. Throw exception is exceptional cases - user not inputting the correct details is NOT an exceptional case. — Arran, Jun 08 '12 at 14:59
I don't want to be picky, but you might consider not displaying the actual exception message to your users. This gives hints to the back end of your application and can give a malicious user hints on ways to attack your system. Instead, log the exception, and give the user a message saying that Ooops something broke. If you log to a SQL table, you can give them the LogID or whatever you are using for a unique key in the DB, just so they can call your help desk with it if they want. — TimWagaman, Jun 08 '12 at 15:30
Obviously valid points. I'm a newb which is also obvious so I was getting my code functional and now I'm looking at it thinking "what the hell was I thinking". I realized that I needed to reformat most of it. The try catch thing is apparently because I don't fully understand when to use them. — marcmiller2007, Jun 08 '12 at 15:36
A `goto`? I thought the velociraptor would have prevented you from posting... — Mike G, Jul 16 '12 at 16:32
yes...a goto. this newb has learned a lot from everyone here. No more displays of ignorance. — marcmiller2007, Jul 17 '12 at 17:31

Adolfo Perez · Answer 1 · 2012-06-08T15:10:31.007

You have too much logic in the button click. Personally I would rather validate on lost focus of each field. If you dont' want to do that then create a method like bool ValidateForm() and wrap all your validation logic there. If at least one inner validation fails then return false. Also create a method like void ClearForm() where you wrap all the logic to clear all fields. Remember that is always good to modularize your code so you can reuse your logic, here you are tying the clear and validation logic to a particular event (button click).

So on Enter Click you want to

if(!ValidateForm())
{
   MessageBox.Show("Invalid Form");
   ClearForm();
   return;
}
SaveForm();

I would highly recommend not using GOTO instructions AT ALL!! This is a very dangerous practice that can turn your code into a spaghetti nightmare.

Hope this helps.

This does help. After coding all that crap to get it functioning I thought it was way too much on the click event that I should send the click event out to another function. — marcmiller2007, Jun 08 '12 at 15:37

score 0 · Answer 2 · edited Jul 16 '12 at 16:28

0

if (MessageBox.Show(Exception.Message) == System.Windows.Forms.DialogResult.OK)
{
    //Clear all needed controls here
}

edited Jul 16 '12 at 16:28

akjoshi

15,374
13
103
121

answered Jun 08 '12 at 14:57

user1162770

3
1
1
2

That answer is a bit terse, maybe this would shed some light:http://support.microsoft.com/kb/816145 – jfenwick Jun 08 '12 at 18:21

score 0 · Answer 3 · answered Jun 08 '12 at 15:00

Lots of problems with your code. Why do you check if ex!=null after you display it in the MessageBox? And using goto labels is not standard in C#.

The nested try is not nice either. Seperate your code a bit, as in have a seperate function that takes care of just validating the data. Your button click handler should not have to do all this work. So you would have a validate() function that takes care of validation logic and a clearForm().

That would essentially untangle all the mess in your button click handler and it would simply have to do something along the lines of:

if (validate())
  //save to db
else
  clearForm()

Also you should rethink your design, function should only throw exception if it is really needed. Do you need to throw an exception because one form input was not in a valid format?

score 0 · Answer 4 · answered Jun 08 '12 at 15:00

0

Just drop the gotos. And let the user fix the wrong entries. When he has done it, he will click on the button again. If you go to restart: you might be caught in an endless loop.

If you clear all the fields when an error occurs, the user will have to reenter all the fields. Just tell him which fields are wrong and why, so that he can just fix the bad entries and can click the button when he fixed the problems.

And by the way, using goto is really ugly. Use it only in very very rare situations!

answered Jun 08 '12 at 15:00

Olivier Jacot-Descombes

104,806
13
138
188

Scractch that, just don't use goto. – Luke Berry Jun 08 '12 at 15:24
@LukeBerry: If you want to break from a nested loop to the end of an enclosing loop, I would say that using a `goto` is legitimate. It is comparable to using a `return` statement. Of cause you can repeat the condition in the outer loop or use a boolean variable, but does it make the code more understandable? – Olivier Jacot-Descombes Jun 08 '12 at 16:09

score 0 · Accepted Answer · edited May 23 '17 at 12:03

0

My suggestion would be use a background worker thread to handle the logic and just have the background worker triggered by button_click. That way you can terminate the background worker thread if the user makes a mistake and restart the whole process via button_click.

Sources:

how to keep a control disabled till a thread ends

http://msdn.microsoft.com/en-us/library/system.componentmodel.backgroundworker.aspx

edited May 23 '17 at 12:03

Community

1
1

answered Jun 08 '12 at 15:51

GeorgePotter

889
1
10
18

If you like it then please upvote it and/or, if you're particularly pleased with this answer, tick it to mark it as the accepted answer for this question. – GeorgePotter Jun 12 '12 at 09:17

button click event C#

5 Answers5