Return to Revisions

I really like how @dagnelies starts. Short and to the point. A good use of high level abstraction. I'm only really only tweaking it's signature (and avoiding a needless negative).

void ParsingTools::filterStrings(QStringList &sl)
{
    for (int i=0; i<sl.length(); i++) {
        QString s = sl.at(i);
        if( isImproperString(s) ) {
           filterStringOut(i);
        }
    }
}

However, I like how @DavidArno breaks out the requirement tests as individual functions. Sure the whole thing becomes longer but every function is wonderfully tiny. Their names avoid the need for comments to explain what they are. I just don't like that they take on the extra responsibility of calling filterStringOut().

By the way, yes C++ will stop evaluation of a || chain on a true so long as you haven't overloaded the || operator. This is called short circuit evaluation.

This should make the definition of an improper string clear without dragging you through needless details:

bool isImproperString(QString s) {
    return isImproperLength(s, m_Length) 
        || sansRequiredSubstring(s, m_Include)
        || hasForbidenSubstring(s, m_Exclude)
    ;
}

Relieved of the need to call filterStringOut() the requirement test functions become shorter and their names are much simpler. Also put everything they're dependant on in their parameter list to make it easier to understand them without looking inside.

bool isImproperLength(QString s, int length) {
    return ( s.length() != length );
}

bool sansRequiredSubstring(QString s, QStringList &include) {
    for (int j=0; j<include.length(); j++) {
        QString requiredSubstring = include.at(j);
        if ( !s.contains(requiredSubstring) ) { 
            return true; 
        }
    }
    return false;
}

bool hasForbidenSubstring(QString s, QStringList &exclude) {
    for (int j=0; j<exclude.length(); j++) {
    QString forbidenSubstring = exclude.at(j);
        if ( s.contains(forbidenSubstring) ) { 
            return true; 
        }
    }
    return false;
}

I added requiredSubstring and forbidenSubstring for the humans. Will they slow you down? Test and find out. It's easier to make readable code actually fast then to make prematurely optimized code readable or actually fast.

If the functions turn out to slow you down look into inline functions before you subject the humans to unreadable code. Again, don't assume this will give you speed. Test.

I think you'll find any of these more readable than nested for loops. Combined with the if's and you were starting to have a real arrow anti-pattern.