Robert Collins wrote:
> Much better. Please supply as a attachment, along with a changelog.
Will do.
> Also, I've made some notes to getReadableCategoryList below that you may
> follow or not at your discression.
Discussing below:
> > +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.
OK. It was a (not so good) attempt at a performance optimization.
> > + 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.
My gut feeling was that an iterator should compare quicker than a string. I
can't see any ill effects to keeping this one. Multiple categories with the same
name aren't going to happen :-)
> 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;
> }
I deliberately tried to minimize the number of operations done in the loop.
Convention vs. a (possibly insignificant) performance gain. Your call.
Advance warning: I've also got a patch for dumpAndList in
IniDBBuilderPackage.cc, which uses the same 'break in middle of while(true)
iteration' technique to achive the same goal (no delimeter at end of list). The
end result is that and list dumps are all on one line, greatly reducing number
of log lines, and also (I think) much more readable. It formats and lists like
this: "foo & baz & baz". If or lists are used (currently they are not), it
formats them as: "foo | ( bar & baz )".
So, if you have a preference for coding style, tell me now, and I will apply it
to that patch too.
> However, I'd probably break this down into smaller chunks still:
...
> 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 :].
Indeed! Stop with the excessive OO!
Max.
