Re: [setup PATCH] Improve Category column (Take 2)

[Robert Collins <rbcollins at cygwin dot com>]

On Fri, 2002-09-20 at 00:57, Max Bowsher wrote:
> Take 2. I'm confident about everything but my const qualifiers on
> packagemeta::getReadableCategoryList ().
> Please pay close attention to them. Thanks.

Much better. Please supply as a attachment, along with a changelog.

Also, I've made some notes to getReadableCategoryList below that you may
follow or not at your discression.

> Comments: Implement packagemeta::getReadableCategoryList ()
> ===================================================================
> RCS file: /home/max/cvsmirror/cygwin-apps-cvs/setup/package_meta.cc,v
> retrieving revision 2.31
> diff -u -p -r2.31 package_meta.cc
> --- package_meta.cc 2002/07/13 14:00:37 2.31
> +++ package_meta.cc 2002/09/19 14:40:13
> @@ -241,6 +241,30 @@ packagemeta::add_category (String const
>    categories.insert (cat);
>  }
> 
> +String const
> +packagemeta::getReadableCategoryList () const
> +{
> +  String catName;
> +  set<String, String::caseless>::const_iterator it = categories.begin();

> +  set<String, String::caseless>::const_iterator end = categories.end();
This is not needed, and can lead to errors because it's only evaluated
the once.

> +  set<String, String::caseless>::const_iterator all = categories.find("All");
this is also not needed, and if we were using something that supports
multiple entries (say a vector) with the same key, would give incorrect
results.

iteration is usually written as a for loop, don't ask me why :}.
so we have
for (set<String, String::caseless>::const_iterator it = set.begin();
  it != set.end(); ++it) {

//  comparing for all is easy
  if (*it == "All")
    continue;

> +    catName += *(it);

the brackets aren't needed - just += *it will do fine.

> +    if (it + 1 == end)
> +      break;
> +    if (all == it + 1) {
> +      continue;
> +    }
> +    catName += ", ";
> +  }
  }  
return catName;
}

However, I'd probably break this down into smaller chunks still:

void 
addCategoryToReadableList (String &theList, String const &aCategory)
{
  if (aCategory == "All")
    return;
  if (theList.size() > 0)
    theList += ", ";
  theList += aCategory;
}

String const
packagemeta::getReadableCategoryList () const
{
  String result;
  for (set<String, String::caseless>::const_iterator it = set.begin();
      it != set.end(); ++it)
    addCategoryToReadableList (result, *it);
}

Which makes about as siple a routine as you can get :}.
Also, not that this then suggests to me that we can factor
addCategoryToReadableList to a ReadableList class, for all our comma
delimited list generation. But hey, one step at a time :].

Rob

Attachment: signature.asc
Description: This is a digitally signed message part