How to avoid break in a loop?

Question

I have a WebBrowser in Windows Forms project. It navigates all links in a table element. It is working fine; however it uses break inside a loop. How to achieve this functionality without break statement?

Note: In my real scenario, all the links will redirect to a login page if we give a Navigate command with that link. So, storing all urls and doing a Navigate afterwards will not work for me, in my actual scenario.

C# Code

public partial class Form1 : Form
{

string websiteUrl = @"C:\Samples_L\MyTableTest.html";
List<string> visitedUrls = new List<string>();
string currentUrl = String.Empty;

private void ExerciseApp(object sender, EventArgs e)
{

    Thread.Sleep(1000);

    if (currentUrl != websiteUrl)
    {
        currentUrl = websiteUrl;
        wb.Navigate(websiteUrl);
    }


    HtmlElement tableElement = wb.Document.GetElementById("four-grid");
    if (tableElement != null)
    {
        foreach (HtmlElement e1 in tableElement.All)
        {
            string x = e1.TagName;
            String idStr = e1.GetAttribute("id");

            if (!String.IsNullOrWhiteSpace(idStr))
            {
                if (idStr.Contains("catalogEntry_img"))
                {
                    string url = e1.GetAttribute("href");
                    if (!visitedUrls.Contains(url))
                    {
                        currentUrl = url;
                        visitedUrls.Add(url);
                        e1.InvokeMember("Click");

                        //Use break when the first match is found
                        break;
                    }
                }
            }
        }
    }

}



private System.Windows.Forms.WebBrowser wb = null;
private Button button1 = null;
private ListBox listBox1 = null;

public Form1()
{

    // button1
    button1 = new Button();
    button1.Location = new Point(20, 430);
    button1.Size = new Size(90, 23);
    button1.Text = "Load and Test";
    button1.Click += new EventHandler(this.button1_Click);

    // listBox1
    listBox1 = new ListBox();
    listBox1.Location = new Point(10, 460);
    listBox1.Size = new Size(460, 200);

    // Web Browser
    wb = new WebBrowser();
    wb.Location = new Point(10, 10);
    wb.Size = new Size(1000, 400);

    //Subscribing for the Document Completed Event
    wb.DocumentCompleted += new WebBrowserDocumentCompletedEventHandler(ExerciseApp);

    // Form1
    this.Text = "Web Browser Test";
    this.Size = new Size(5000, 7100);
    this.Controls.Add(wb);
    this.Controls.Add(button1);
    this.Controls.Add(listBox1);

    currentUrl = websiteUrl;
}

private void button1_Click(object sender, EventArgs e)
{
    listBox1.Items.Add("Loading Web app under test into WebBrowser control");
    wb.Url = new Uri(websiteUrl);

}


}

HTML Used

<html>
<head>

    <style type="text/css">
        table {
            border: 2px solid blue;
        }

        td {
            border: 1px solid teal;
        }
    </style>

</head>
<body>

    <table id="four-grid">
        <tr>
            <td>
                <a href="https://stackoverflow.com/users/696627/lijo" id="catalogEntry_img63664" class="itemhover"
                    onfocus="showPopupButton('category_63664');"
                    onkeydown="shiftTabHidePopupButton('category_63664',event);">

                    <img src="ssss"
                        alt="G" width="70" />
                </a>
            </td>
            <td>
                <a href="http://msdn.microsoft.com/en-US/#fbid=zgGLygxrE84" id="catalogEntry_img63665" class="itemhover"
                    onfocus="showPopupButton('category_63665');"
                    onkeydown="shiftTabHidePopupButton('category_63665',event);">

                    <img src="ssss"
                        alt="Y" width="70" />
                </a>
            </td>
        </tr>
        <tr>
            <td>
                <a href="https://www.wikipedia.org/" id="catalogEntry_img63666" class="itemhover"
                    onfocus="showPopupButton('category_63666');"
                    onkeydown="shiftTabHidePopupButton('category_63666',event);">

                    <img src="ssss"
                        alt="B" width="70" />
                </a>
            </td>
            <td>
                <a href="http://www.keralatourism.org/" id="catalogEntry_img63667" class="itemhover"
                    onfocus="showPopupButton('category_63667');"
                    onkeydown="shiftTabHidePopupButton('category_63667',event);">

                    <img src="ssss"
                        alt="A" width="70" />
                </a>
            </td>
        </tr>
    </table>
</body>

</html>

Reference

Next using LINQ approach

Iterate yourself instead of using foreach, and keep a bool variable that equals true when you have to break. In the loop condition, check that you have more elements and that this bool variable is still false — Julián Urbano, Mar 26 '14 at 15:35
Reference [Is it a bad practice to use break in a for loop?](http://stackoverflow.com/questions/3922599/is-it-a-bad-practice-to-use-break-in-a-for-loop) — LCJ, Mar 26 '14 at 15:42
@Lijo Your example only has one construct that will respond to the `break` so I don't see the need to refactor based on removing it, however, @Servy's answer shows another reason to refactor: readability and expression of intent. — Adam Houldsworth, Mar 26 '14 at 15:45
Except that the highest voted answers in your reference all say that using `break` or `continue` is just fine. I don't understand why you would want to make the code _harder_ to read & maintain to satisfy some weird academic idea. — Craig W., Mar 26 '14 at 15:45
@CraigW. Perhaps the reference was to the part about multiple nested sub-blocks or length of code. — Adam Houldsworth, Mar 26 '14 at 15:49
I also don't see the purpose of removing the break. But the one change I would make, for readability, is to reverse the logic on your `if` statements and use `continue`. So instead of `if (meetsCondition) { DoDtuff }`, you have `if (DoesntMeetCondition) continue; DoStuff;` This reduces nesting. Other than that, it looks fine to me... — HiredMind, Mar 26 '14 at 16:10

score 4 · Accepted Answer · answered Mar 26 '14 at 15:39

4

You could refactor it into a query, making the intentions of the code somewhat clearer:

var nextElement = tableElement.All
    .Where(element => element.GetAttribute("id") != null &&
        element.GetAttribute("id").Contains("catalogEntry_img") &&
        !visitedUrls.Contains(element.GetAttribute("href")))
    .FirstOrDefault();

if(nextElement != null)
{
    visitedUrls.Add(nextElement.GetAttribute("href"));
    nextElement.InvokeMember("Click");
    currentUrl = nextElement.GetAttribute("href");
}

I'd also suggest changing visitedUrls to a HashSet, rather than a list, as it is a more efficient data structure for simply determining if an item is in a set of items.

answered Mar 26 '14 at 15:39

Servy

202,030
26
332
449

This also uses break - it just hides it :) – Danvil Mar 26 '14 at 15:40
1

@Danvil [No, actually it doesn't.](http://referencesource.microsoft.com/#System.Core/System/Linq/Enumerable.cs#8087366974af11d2) And even if it did, that wouldn't be all that relevant. You're not responsible for maintaining `FirstOrDefault`. MS is going to ensure that it works. Under the hood all loops use a GOTO. That's not wrong, but that doesn't mean you should be using GOTO. When building tools/layers of abstraction you use low level tools *so that other code can just use the high level tools*, and avoid the headache of using the low level tools. – Servy Mar 26 '14 at 15:42
1

@Danvil Who care? The compiler uses `goto`, but hides it. The important bit being "hides it". Even if `break` is still used, but hidden, the perceived confusion it creates (or whatever argument the OP is using) is removed because it has been abstracted. – Adam Houldsworth Mar 26 '14 at 15:46

score 0 · Answer 2 · answered Mar 26 '14 at 15:45

If you iterate over a range of values and want to exit as soon as you found one which satisfies a condition, it is common to use break:

foreach(var a in list) {
   if(test(a)) {
       // use a
       break;
   }
}

Sometimes it may be undesired and you can just use this:

bool found = false;
foreach(var a in list) {
   if(found && test(a)) {
       // use a
       found = true;
   }
}

or even:

bool found = false;
foreach(var a in list) {
   if(test(a)) {
       if(found) {
           // use a
           found = true;
       }
   }
}

The first version can skip iterating once a suitable element is found. The other two variants keep iterating or even checking all elements. In almost all cases this just wastes processing power. But have a look here for a case where you actually might want to use the third variant: http://en.wikipedia.org/wiki/Timing_attack

How to avoid break in a loop?

2 Answers2