Re: continuing continue - was Re: [patch] continue statement

[Glenn Maynard <glenn@...>]

On Sat, Sep 24, 2005 at 07:27:16PM -0500, Rici Lake wrote:
> In any event, since the above does not appear to be in a loop, a 
> "continue" statement isn't going to help (although "try...finally" 
> would reduce verbiage quite a bit.)

It was a minor tangent: the example I gave was for conditionals, but
most of the loop alternatives you were giving were in a similar style,
and can lead to the same problem.  (The "cleanup" case is one of the
reasons people sometimes write deep conditionals like that, but that
wasn't the case in the code I was reading.)

> In other words, out of about six million lines of C (including 
> comments), "break" statements account for about 1.5% and "continue" 
> statements account for about 0.2%; of the almost 11,000 files, slightly 
> more than half have at least one break statement, but slightly less 
> than a quarter have a continue statement (and of those, more than half 
> have only one or two). Out of curiosity, I also checked:

One well-placed break or continue can cause a dozen lines of code to
lose a level of nesting.  A quick search in my project file finds 142
files that use continue and 204 that use break; cleaning up (by my
perceptions, at least) code in a few hundred files I help maintain is
significant to me.

Another one: it's easier to debug, in most debuggers (though this is
currently mostly moot in Lua, with few good debuggers available anyway).
It's easier to step through:

  if(a())
    continue;
  if(b())
    continue;
  if(c())
    continue;

then

  if( !a() && !b() && !c() )
   ...

and I can put breakpoints on the continues, too, which I do very often.

I've attached (due to length) a slightly more significant real-world example:
a loop with a break and ten continues.  If I tried, I could find a
maintainable way to do without them, but a lot of time is saved by writing
things naturally.  :)

>        if (spidx) {
>                LIST_FOREACH(sp, &sptree[dir], chain) {
>                        if (sp->state != IPSEC_SPSTATE_DEAD && 
> sp->spidx) {

I agree with moving the "spidx" conditional out of the loop, but I much
prefer the IPSEC_SPSTATE_DEAD written with continue.

-- 
Glenn Maynard

	//
	// Now, For the images we still haven't found, look at the image dimensions of the remaining unclassified images.
	//
	CStringArray arrayImages;
	GetImageDirListing( m_sSongDir + "*", arrayImages );

	for( unsigned i=0; i<arrayImages.size(); i++ )	// foreach image
	{
		if( HasBanner() && HasCDTitle() && HasBackground() )
			break; /* done */

		// ignore DWI "-char" graphics
		if( BlacklistedImages.find( arrayImages[i].c_str() ) != BlacklistedImages.end() )
			continue;	// skip
		
		// Skip any image that we've already classified

		if( HasBanner()  &&  stricmp(m_sBannerFile, arrayImages[i])==0 )
			continue;	// skip

		if( HasBackground()  &&  stricmp(m_sBackgroundFile, arrayImages[i])==0 )
			continue;	// skip

		if( HasCDTitle()  &&  stricmp(m_sCDTitleFile, arrayImages[i])==0 )
			continue;	// skip

		CString sPath = m_sSongDir + arrayImages[i];

		/* We only care about the dimensions. */
		CString error;
		RageSurface *img = RageSurfaceUtils::LoadFile( sPath, error, true );
		if( !img )
		{
			LOG->Trace( "Couldn't load '%s': %s", sPath.c_str(), error.c_str() );
			continue;
		}

		const int width = img->w;
		const int height = img->h;
		delete img;

		if( !HasBackground()  &&  width >= 320  &&  height >= 240 )
		{
			m_sBackgroundFile = arrayImages[i];
			continue;
		}

		if( !HasBanner() && Sprite::IsDiagonalBanner(width, height) )
		{
			m_sBannerFile = arrayImages[i];
			continue;
		}

		if( !HasBanner()  &&  100<=width  &&  width<=320  &&  50<=height  &&  height<=240 )
		{
			m_sBannerFile = arrayImages[i];
			continue;
		}

		/* Some songs have overlarge banners.  Check if the ratio is reasonable (over
		 * 2:1; usually over 3:1), and large (not a cdtitle). */
		if( !HasBanner() && width > 200 && float(width) / height > 2.0f )
		{
			m_sBannerFile = arrayImages[i];
			continue;
		}

		/* Agh.  DWI's inline title images are triggering this, resulting in kanji,
		 * etc., being used as a CDTitle for songs with none.  Some sample data
		 * from random incarnations:
		 *   42x50 35x50 50x50 144x49
		 * It looks like ~50 height is what people use to align to DWI's font.
		 *
		 * My tallest CDTitle is 44.  Let's cut off in the middle and hope for
		 * the best. */
		if( !HasCDTitle()  &&  width<=100  &&  height<=48 )
		{
			m_sCDTitleFile = arrayImages[i];
			continue;
		}
	}