locked
GOTO RRS feed

  • Question

  • User1080785583 posted

    how can i fix this GOTO statement? I try to refactor it and the loops breaks and I am so confused by this logic!!!!!!!!!!!

    foreach (XmlNode node in pageControls)
                {
    // removed about 10000 lines of more case gotos
    
    switch (type)
                    {
                        case "TextBox":
                            {
                                var box5 = (TextBox)callerPage.FindControl(element);
                                box5.Text = strDefaultValue;
                                box5.ReadOnly = isReadOnly == "Yes";
                                if (maxLength != "")
                                {
                                    box5.MaxLength = int.Parse(maxLength);
                                }
                                if (strMaxSize != "")
                                {
                                    box5.Attributes.Add("size", strMaxSize);
                                }
                                webControl = box5;
                                goto Label_20D8;
                            }
    }
    
    
                    Label_205C:
                    if (isDisabled == "Yes")
                    {
                        var file4 = (HtmlInputFile)callerPage.FindControl(element);
                        var label8 = (Label)callerPage.FindControl("cap" + element.Substring(3));
                        if (label8 != null)
                        {
                            label8.Text = strCaption != "" ? strCaption : label8.Text;
                        }
                        file4.Attributes.Add("Disabled", "true");
                    }
                    Label_20D8:
                    setControlAttributes(webControl, node, pageLangCode, rowTotalControl, dependentNode, strTemplate);
                    GenerateViewModel(node, pageLangCode, strTemplate, blChildTemp, blParentTemp, sbChildViewModel,
                        sbViewModel,
                        sbParentViewModel);
                    Label_InstallmentGrid:
                    if (type.ToUpper() == "INSTALLMENTGRID")
                    {
                        num++;
                        child.Controls.Add(CreateInstallMentGrid(element, node));
                    }
    } //End Of foreach loop

    Thursday, March 3, 2016 2:45 PM

Answers

  • User281315223 posted

    There is a reason that you don't see the goto keyword used often and this question is a perfect example of that. It often creates "spaghetti code" that can make it incredibly difficult to determine the proper line of execution for a developer reading through it.

    Each of the individual functions within the code you provided look quite different. I was going to suggest if they look at all similar, you could consider breaking them out into their own methods and calling those in some type of order.

    The only real way I can think of tackling this would be to build a long chain that connects your goto statement to the code that precedes it. After or during this process, determine if there are any of these calls that could be reused (e.g. are there multiple goto calls that point to the same label?) if so, refactor those into methods that can be called separately to clean up the code.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, March 3, 2016 4:58 PM
  • User303363814 posted

    Remove the code after the label and put it in its own method, you may need a whole lotta parameters depending on the scope of the variables that are referenced.

    To do this, in VS2015, select the code and type control-. (or right-click, 'quick action') then select 'Extract method'. In older versions of VS I think there was a right click menu option called 'refactor' to help with this.

    If you use the 'Extract method' then a call to the new method will be inserted in you code.  If you don't use the automatic tool then you will need to insert the call yourself.

    Everything should still work at this stage.

    Replace the GOTO with a call to the method followed by a break statement.

    Everything should still work.

    Finally, remove the label.  You could search for any remaining references to the label before deleting it or just let the compile find them for you.  Either way, any other references to the label will need to be replace with appropriate calls to the new method.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, March 3, 2016 10:59 PM

All replies

  • User281315223 posted

    There is a reason that you don't see the goto keyword used often and this question is a perfect example of that. It often creates "spaghetti code" that can make it incredibly difficult to determine the proper line of execution for a developer reading through it.

    Each of the individual functions within the code you provided look quite different. I was going to suggest if they look at all similar, you could consider breaking them out into their own methods and calling those in some type of order.

    The only real way I can think of tackling this would be to build a long chain that connects your goto statement to the code that precedes it. After or during this process, determine if there are any of these calls that could be reused (e.g. are there multiple goto calls that point to the same label?) if so, refactor those into methods that can be called separately to clean up the code.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, March 3, 2016 4:58 PM
  • User303363814 posted

    Remove the code after the label and put it in its own method, you may need a whole lotta parameters depending on the scope of the variables that are referenced.

    To do this, in VS2015, select the code and type control-. (or right-click, 'quick action') then select 'Extract method'. In older versions of VS I think there was a right click menu option called 'refactor' to help with this.

    If you use the 'Extract method' then a call to the new method will be inserted in you code.  If you don't use the automatic tool then you will need to insert the call yourself.

    Everything should still work at this stage.

    Replace the GOTO with a call to the method followed by a break statement.

    Everything should still work.

    Finally, remove the label.  You could search for any remaining references to the label before deleting it or just let the compile find them for you.  Either way, any other references to the label will need to be replace with appropriate calls to the new method.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, March 3, 2016 10:59 PM
  • User1080785583 posted

    go to within a foreach loop within a case that falls into other cases is something of an abomination

    Tuesday, March 15, 2016 6:41 PM