[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: The no goto religion
From:       "Kalle Last" <kalle.last () gmail ! com>
Date:       2007-08-01 15:52:29
Message-ID: 263dcf060708010852x3d66a56fue74d33ca02683d08 () mail ! gmail ! com
[Download RAW message or body]

I'm no KDE developer but I have been watching some of the lists for
some time now and I have an alternative view.


Let's take the original code:
> ----------------
>
> KIcon icon;
>
> for ( KIconThemeNode *themeNode = d->links.first() ; themeNode ;
>          themeNode = d->links.next() )
> {
>      icon = themeNode->theme->iconPath(file, size, KIcon::MatchExact);
>      if (icon.isValid())
>          break;
> }
>
> if ( !icon.isValid() )
> {
>      for ( KIconThemeNode *themeNode = d->links.first() ; themeNode ;
>              themeNode = d->links.next() )
>      {
>          icon = themeNode->theme->iconPath(file, size, KIcon::MatchBest);
>          if (icon.isValid())
>              break;
>      }
> }
>
> file = icon.isValid() ? icon.path : QString::null;
>
> ----------------

First thing I see here is two nearly identical blocks of code. Why not
put that code into a separate function? Searching an icon should be
non-trivial task and function call overhead will be minimal. Also it
would probably save a bit on memory usage thanks to removing some of
the code.

So here is what I propose:
____
KIcon findIcon(file, size, matchType){
	KIcon icon;
	for ( KIconThemeNode *themeNode = d->links.first() ; themeNode ;
		themeNode = d->links.next() )
	{
		icon = themeNode->theme->iconPath(file, size, matchType);
		if (icon.isValid())
			break;
	}
	return icon;
}


/// later in code

KIcon icon=findIcon(file, size, KIcon::MatchExact);

if (!icon.isValid){
	findIcon(file, size, KIcon::MatchBest);
}

file = icon.isValid() ? icon.path : QString::null;
____


Result seems to be shorter and much simple to understand. Only thing
left is to hope it works as it is supposed to be. I'd like to see
someone suggesting a goto with this code :)

-- 
Kalle Last
 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]