Thread: simple while loop

Originally Posted by abachler

using a lot of breaks inside an infinite while loop is just obfuscating the fact that you are using a goto loop, only a less readable one, since at least with goto you can 'find' for the label, whereas with while-break you have to manually search for the end of context.

But that's why there are braces. You can easily see where a break will break the loop. Unless you're referring to something entirely different, I don't see how that is?

> Such as the body

Later perusal of the code can be problematic at points if the one doing the reading expects much of the logic of the loop condition to be defined on the header (as it should) and instead finds rules to be broken, altered or new ones being defined in the body.

It's a matter of visibility just as CornedBee pointed out. Naturally breaks in a loop can be useful, but the context of this debate is that of breaks being placed where they could be replaced by a proper heading simply because the programmer is too confused (or lazy) by a complex conditional expression.

In that context, breaks don't add anything. On the contrary they remove one important aspect; code readability. In the presence of the break, the reader still has to evaluate that new expression against the loop condition, just like they would do if it had been inserted in the loop header, sometimes several lines up in the code which doesn't help our already poor short-term memory. On the other hand, skimming through the code may result in missing the break altogether, missinterpreting it or erroneously evaluating its importance.

EDIT: many complex logical expressions can be made easier to read by proper use of parenthesis and... lord, I'm sorry for what I'm about to say... commenting the code.

From what I see, if I see a break, I can think "oh, so if str == x, then the loop will break." Right there.
If I see str != x as the condition, I need to scan the loop to see where that condition might occur, so I define it as more complex than breaks. For single expressions, it's alright, but when you get multiple expressions, it start getting complicated.
As aside from when I see a break, I know that if x == NULL at this point, it will break, regardless of any other conditions. But if it's in the header, then I must evaluate, does the other condition also hold so that the loop will break?

> As aside from when I see a break, I know that if x == NULL at this point, it will break, regardless of any other conditions. But if it's in the header, then I must evaluate, does the other condition also hold so that the loop will break?

You will still have to evaluate the break expression against the header if you are bug-hunting, improving code performance (speaking of which you may want to look at your compiler documentation regarding this issue), or doing any other kind of code maintenance. Why else, would you want to look at old code?

Another good thing with break in body, is that I can easily control the conditions instead of thinking up a good loop condition such as, I need to make sure it breaks if this and this happens, but not that and then map that so it works with the code inside the loop.
With breaks it's straight forward since you know if x == "1" at this point, then it must break.
When writing some loops, I have no idea of what the condition must be, so I write an infinite loop and add breaks. Typically, if I wanted head conditions, I would have to go over that code and map it into a good header condition.

Originally Posted by Mario F.

You will still have to evaluate the break expression against the header if you are bug-hunting

I never really have a clear image of what the head condition is. I always use the break-approach so I know it's going to break at this point. So I just need to evaluate the break expression to see if it's correct or not. Makes bug-hunting easier than complex head conditions.

...improving code performance (speaking of which you may want to look at your compiler documentation regarding this issue)

Now that's unfortunately the drawback with the break-approach. But then I would use a profiler to see the bottlenecks and ponder how to optimize it.

...or doing any other kind of code maintenance.

Maintenance is much easier with the break-approach

I shudder to imagine the kind of loop you write that you found your approach easier. Care to post an example?

Hmmm. There may be more examples, but this one might do for a start. I shudder to think what would happen if I tried to make it header condition based. It's probably not possible:

Code:

for(;;)
{
	if (bCancel)
	{
		bCountCancel = true;
		bFindCancel = true;
		for(;;) // Must wait until threads complete
		{
			if (bCountFinished && bFindFinished) break;
			Sleep(100);
		}
		DoCancel(i, pFiles);
		goto Exit;
	}
	if (!bCountFinished && !bFindFinished) // None of threads are finished
	{
		CString strFormat;
		Stuff::RequestData::CountFiles();
		strFormat.Format(_T("Searching for files in the specified directory. Please wait. So far, %I64u files has been found."), nTotalFiles);
		m_Searching.SetWindowText(strFormat);
		Sleep(500);
		continue;
	}
	if (bCountFinished)
	{
		CString strFormat;
		strFormat.Format(_T("Collecting files. Please wait. %I64u files were found."), nTotalFiles);
		m_Searching.SetWindowText(strFormat);
		Stuff::RequestData::FindFiles();
		m_fProgress = (float)nCurrentFile / nTotalFiles * 100;
		Sleep(100);
	}
	if (bFindFinished) break;
}

I'll see if I can find any better examples.

Holy spaghetti code, Batman!

Well, you could start by doing

Code:

while(!bFindFinished) ...

That would already greatly increase the readability for me.

Then you could add && !bCancel to the condition and put the actual cancel handling code after the loop.

Then you could split the loop into two parts, one for bCountFinished and one for not.

Of course, with all the threading issues and global variables and polling, I'd just throw the entire thing away and write it from scratch.


Anyway, this confirmed my suspicions. Basically, you want the loop to do too much, which is why you have such complex situations.

How about using do while for this:

Code:

do
{
	if (bCancel)
	{
		bCountCancel = true;
		bFindCancel = true;
                while(!(bCountFinished && bFindFinished))
		{
			Sleep(100);
		}
		DoCancel(i, pFiles);
		goto Exit;
	}
	if (!bCountFinished && !bFindFinished) // None of threads are finished
	{
		CString strFormat;
		Stuff::RequestData::CountFiles();
		strFormat.Format(_T("Searching for files in the specified directory. Please wait. So far, %I64u files has been found."), nTotalFiles);
		m_Searching.SetWindowText(strFormat);
		Sleep(500);
	} else if (bCountFinished)
	{
		CString strFormat;
		strFormat.Format(_T("Collecting files. Please wait. %I64u files were found."), nTotalFiles);
		m_Searching.SetWindowText(strFormat);
		Stuff::RequestData::FindFiles();
		m_fProgress = (float)nCurrentFile / nTotalFiles * 100;
		Sleep(100);
	}
} while(!bCountFinished);

Also changed a few other things, marked in red.

--
Mats

Originally Posted by CornedBee

Anyway, this confirmed my suspicions. Basically, you want the loop to do too much, which is why you have such complex situations.

I agree with that.

It may well _DO_ what you want, but it's not very nicely organized. And if I understand it right, the only reason you do all that in ONE loop, rather than two [one for counting, one for inserting] is that you want common cancellation - so perhaps you should put the cancel stuff in a function, and call that from either of the two loops?

--
Mats

Originally Posted by CornedBee

Holy spaghetti code, Batman!

I know, I know. I typically just write the loop and don't give a crap about re-designing it later by looking over it and structuring a little better perhaps. A bad habit

Of course, with all the threading issues and global variables and polling, I'd just throw the entire thing away and write it from scratch.

I don't think I'd agree with that really. I've given it much thought.
Tightly responsive GUI is very important to me, and I therefore believe that being able to cancel at any time is very important. I just hate those applications that seem to take an eternity to complete, freeze the GUI, give no choice to cancel, or just sits there eating up CPU and shows no visual feedback and when clicking cancel it just doesn't respond.

Perhaps the blocking until the threads are complete could be put into a separate function, though. I just need to make sure they finish so I can kill the buffer I pass to it (since it's threaded!).

Otherwise, I find the polling very ingenious. It will allow me to skip expensive locking and just a flag. If the flag is set, it will update progress into the correct variables and block until it's finished. So no race issues.

No, the polling is exactly the problem. Notifications should be pushed, not polled. For example, you shouldn't wait for the threads to finish by polling a global variable (without synchronization - is it a special atomic type?). You should do it by calling WaitForMultipleObjects(). Either on the threads themselves or, if they don't die, on event objects that get signalled when the threads are done.
Actually you shouldn't loop at all. Rather, you should return to the message loop, and the threads should PostMessage() notifications whenever they've done a part of their job. You can update the UI in response. The same goes for cancellation, actually.

Originally Posted by CornedBee

No, the polling is exactly the problem. Notifications should be pushed, not polled. For example, you shouldn't wait for the threads to finish by polling a global variable (without synchronization - is it a special atomic type?). You should do it by calling WaitForMultipleObjects(). Either on the threads themselves or, if they don't die, on event objects that get signalled when the threads are done.

Here is how I did the polling:

Code:

namespace RequestData
{

void CountFiles() 
{
	ResetEvent(heRequestData_CountFiles);
	bRequestData_CountFiles = true;
	DWORD dw = WaitForSingleObject(heRequestData_CountFiles, 5000);
	ASSERT(dw == WAIT_OBJECT_0);
	//ResetEvent(heRequestData_CountFiles);
}

void FindFiles() 
{
	ResetEvent(heRequestData_FindFiles);
	bRequestData_FindFiles = true;
	DWORD dw = WaitForSingleObject(heRequestData_FindFiles, 5000);
	ASSERT(dw == WAIT_OBJECT_0);
	//ResetEvent(heRequestData_FindFiles);
}

}

The idea is that the application should notify the threads that it wants the data (avoiding expensive locking and filling the buffers all the time), and so it sets resets an event, sets a flag to tell it wants the data and then waits for the event to be set again.
The thread code itself will raise the event when it's done filling the information.

Actually you shouldn't loop at all. Rather, you should return to the message loop, and the threads should PostMessage() notifications whenever they've done a part of their job. You can update the UI in response. The same goes for cancellation, actually.

That's a good principle. However, I'm not sure how to actually implement it. The FindFiles function will loop and not provide any progress until I explicitly ask for it (for speed, for one thing). So in any case, there must be some code somewhere that tells it to update progress and perhaps send a message to the window.

Originally Posted by matsp

How about using do while for this:
Also changed a few other things, marked in red.

You may be right on that it works. I made a few changes. You're right that when looking at the loop again, a few things could easily be used as a head condition.

Originally Posted by tabstop

What about this?
I think this works identically, but I didn't break out the formal proof.

Your code is pretty much identical to what mats suggested except the cancel and goto. I put the goto there because it jumps to a common branch at the end of the function and does cleanup before returning control to the user and the GUI. That's why I use the goto. If you would just break the loop with cancel, I still have to do the cleanup or preparation and possible do a goto later instead.