goto in c# and its usage

Question

I have a subroutine. It comapares whether values are empty then doing something. For example, if they are empty, then warnings are raised. The code works fine. But when value are not empty, the warnings are still pop out. Please help me to correct the logic.

Thanks.

private void btnNew_Click(object sender, EventArgs e)
    {
        try
        {
            if (txtbox1.Text.ToString().Trim() == string.Empty)
            {
                goto Msg1;
            }
            if (txtbox2.Text.ToString().Trim() == string.Empty)
            {
                goto Msg2;
            }
            DataRow dr = mydataSet.Tables[0].NewRow();
            dr["Descript"] = txtbox1.Text;
            dr["Abbr"] = txtbox2.Text;
            dr["SortOrder"] = Convert.ToDecimal(numericOrder.Value);
            if (SortOrders.Contains((decimal)dr["SortOrder"]))
            {
                goto Msg3;
            }
            mydataSet.Tables[0].Rows.Add(dr);
            dgv.DataSource = mydataSet.Tables[0];
        Msg1:
            MessageBox.Show("Description is required.");
        Msg2:
            MessageBox.Show("Abbr is required.");
        Msg3:
            MessageBox.Show("Please select another one, this one is already used.");
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
        }
    }

From the above code, you see. if txtbox1 has some value, the program still displays Msg1. I want to avoid it.

No need to call `ToString()` on a `string` -- why not just `txtbox1.Text.Trim()` and so on? — phoog, Jul 30 '12 at 21:23

score 9 · Accepted Answer · edited Jul 30 '12 at 21:22

Because labels are just labels, and code after them is executed sequentially.

Why can't you do just this:

    try
    {
        if (txtbox1.Text.ToString().Trim() == string.Empty)
        {
            MessageBox.Show("Description is required.");
            return;
        }
        if (txtbox2.Text.ToString().Trim() == string.Empty)
        {
            MessageBox.Show("Abbr is required.");
            return;
        }
        DataRow dr = mydataSet.Tables[0].NewRow();
        dr["Descript"] = txtbox1.Text;
        dr["Abbr"] = txtbox2.Text;
        dr["SortOrder"] = Convert.ToDecimal(numericOrder.Value);
        if (SortOrders.Contains((decimal)dr["SortOrder"]))
        {
            MessageBox.Show("Please select another one, this one is already used.");
            return;
        }
        mydataSet.Tables[0].Rows.Add(dr);
        dgv.DataSource = mydataSet.Tables[0];
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message);
    }

It's so much more readable.

score 4 · Answer 2 · answered Jul 30 '12 at 21:03

Restructure your code to avoid goto - it is a relic and not much use in a properly object oriented codebase.

Returning from the method, throwing exceptions or building an errors dictionary are all better options than using goto.

For example, you can have a List<string> errors which you add to when you get an error condition.

If it is empty, no errors were encountered, if it isn't, there were.

Karl-Johan Sjögren · Answer 3 · 2012-07-30T21:13:26.050

This is a good case were goto is the wrong way to go. Use something like this instead.

private void btnNew_Click(object sender, EventArgs e)
{
    try
    {
        bool error = false;
        if (txtbox1.Text.ToString().Trim() == string.Empty)
        {
            MessageBox.Show("Description is required.");
            error = true;
        }
        if (txtbox2.Text.ToString().Trim() == string.Empty)
        {
            MessageBox.Show("Abbr is required.");
            error = true;
        }
        if (SortOrders.Contains(Convert.ToDecimal(numericOrder.Value)
        {
            MessageBox.Show("Please select another one, this one is already used.");
            error = true;
        }

        if(error)
            return;

        DataRow dr = mydataSet.Tables[0].NewRow();
        dr["Descript"] = txtbox1.Text;
        dr["Abbr"] = txtbox2.Text;
        dr["SortOrder"] = Convert.ToDecimal(numericOrder.Value);
        mydataSet.Tables[0].Rows.Add(dr);
        dgv.DataSource = mydataSet.Tables[0];
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
}

Edit

Just figured that my code didn't actually do the same as his first sample since it only displayed the first error no matter how many that occured. Updated my sample to accomodate for that.

As always :) But I noticed an error in my code (and the other sample by Serg) so I updated mine a bit. — Karl-Johan Sjögren, Jul 30 '12 at 21:14

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

I've always been taught to avoid goto like the plague, and it's something I've followed for years. I've never even considered it to be an option when writing code.

Thinking about it though, I did read an article a few years ago (which I can't find now) which said you could credibly use gotos only if you used it to jump down code, and not up: a rule that is stuck to here.

Check here for more info: Does anyone still use [goto] in C# and if so why?

There are better ways of using goto statement, for instacne using "return" (when used in the middle of a method), "break" and "continue". Have you ever used one of these?

score 0 · Answer 5 · answered Jul 30 '12 at 21:10

private void btnNew_Click(object sender, EventArgs e)
{
    try
    {
        var description = txtbox1.Text.Trim();
        if (string.IsNullOrEmpty(description))
        {
            MessageBox.Show("Description is required.");
            return;
        }
        var abbr = txtbox2.Text.Trim();
        if (string.IsNullOrEmpty(abbr))
        {
            MessageBox.Show("Abbr is required.");
            return;
        }
        var numericOrderValue = Convert.ToDecimal(numericOrder.Value);
        if (SortOrders.Contains(numericOrderValue)
        {
            MessageBox.Show("Please select another one, this one is already used.");
            return;
        }

        DataRow dr = mydataSet.Tables[0].NewRow();
        dr["Descript"] = description;
        dr["Abbr"] = abbr;          
        dr["SortOrder"] = numericOrderValue;            
        mydataSet.Tables[0].Rows.Add(dr);
        dgv.DataSource = mydataSet.Tables[0];
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
}

score -1 · Answer 6 · answered Jul 30 '12 at 21:06

private void btnNew_Click(object sender, EventArgs e)
    {
        try
        {
            if (txtbox1.Text.ToString().Trim() == string.Empty)
            {
                MessageBox.Show("Description is required.");
            }
            if (txtbox2.Text.ToString().Trim() == string.Empty)
            {
                MessageBox.Show("Abbr is required.");
            }
            DataRow dr = mydataSet.Tables[0].NewRow();
            dr["Descript"] = txtbox1.Text;
            dr["Abbr"] = txtbox2.Text;
            dr["SortOrder"] = Convert.ToDecimal(numericOrder.Value);
            if (SortOrders.Contains((decimal)dr["SortOrder"]))
            {
               MessageBox.Show("Please select another one, this one is already used.");
            }
            mydataSet.Tables[0].Rows.Add(dr);
            dgv.DataSource = mydataSet.Tables[0];

        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
        }
    }

Try this. It works.

It "works", but you will still be adding rows if there is invalid data so I would say that it is broken. — Karl-Johan Sjögren, Jul 30 '12 at 21:16

goto in c# and its usage

6 Answers6

Edit