Angus Leeming <[EMAIL PROTECTED]> writes:
| This is going to bomb unless you add a return statement
| +void BranchList::setColor(string const & s, string const & val)
| +{
| + List::iterator it = list.begin();
| + List::iterator end = list.end();
| + for (; it != end; it++) {
Use ++it, instead of it++.
| + if (s.find(it->getBranch(), 0) != string::npos) {
| + it->setColor(val);
| return;
| + }
| + }
| + Assert(false);
| +}
|
|
| Presumably, this one would benefit from either a 'return' or a 'break'
| +void BranchList::setSelected(string const & s, bool val)
| +{
| + List::iterator it = list.begin();
| + List::iterator end = list.end();
| + for (; it != end; it++) {
ditto
| + if (s.find(it->getBranch(), 0) != string::npos) {
| + it->setSelected(val);
| return
| }
| + }
| +}
|
|
| Here you should move the (j == string::npos) test to immediately after the j
| = s.find_first_of line, else the very next line is going to be nasty.
| +void BranchList::add(string const & s)
| +{
| + string name;
only used in loop?
| + string::size_type i = 0;
this too?
| + Branch br;
this too?
| + for (;;) {
A bit too C-like to me. I prefere while (true), matter of taste I
guess.
| + string::size_type const j = s.find_first_of(separator(), i);
| + name = s.substr(i, j - i);
| + br.setBranch(name);
| + br.setSelected(false);
| + br.setColor("none");
| + list.push_back(br);
| + if (j == string::npos)
| + break;
| + i = j;
| + i++;
ditto
but why not "i = j + 1"?
| + }
| +}
|
|
| Does it make sense to setColor here if j == npos?
| +void BranchList::setColors(string const & s)
| +{
| + string name;
is name only used in the loop? move delaration there.
| + string::size_type i = 0;
| + List::iterator it = list.begin();
| + for (;;) {
and here.
Why isn't i part of the loop setup?
(ah.. i = j)
What about it?
| + string::size_type const j = s.find_first_of(separator(), i);
| + name = s.substr(i, j - i);
| + it->setColor(name);
| + if (j == string::npos)
| + break;
| + it++;
ditto
| + Assert(it != list.end());
| + i = j;
| + i++;
ditto
but why not "i = j + 1"?
--
Lgb
