Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This program will filter the input by replacing matching bad words with "Bleep!". I'd like the code to be more concise and C++ style where possible. One thing that bugs me is the was_bad flag I think is needed to skip printing a word that has matched one of my bad words - if there was some better way to skip the rest of the while loop upon encountering a bad word so I don't print Bleep! poop, for instance.

#include <iostream>
#include <vector>
#include <string>

using namespace std;

int main(void) 
{
    int i, was_bad = false;
    string input, bad[] = {"poop", "balls"};
    vector <string> badwords(bad, bad + sizeof(bad) / sizeof(string));

    while (cin >> input)
    {
        for (i = 0; i < badwords.size(); ++i)
            if (badwords[i] == input)
            {
                cout << "Bleep! ";
                was_bad = true;
                break;
            }

        if (!was_bad)
            cout << input << " ";

        was_bad = false;
    }

    return 0;
}

EDIT: One thing that did strike me was to use a ternary operator:

while (cin >> input)
{
    for (i = 0; i < badwords.size(); ++i)
        if (badwords[i] == input)
        {
            is_bad = true;
            break;
        }

    cout << (is_bad ? "Bleep! " : input + " ");

    is_bad = false;
}
share|improve this question
    
so I can't say poop, but I can say POOP and pooop p00p I would recommend looking at c++11 regular expressions. cplusplus.com/reference/std/regex –  Xploit Nov 16 '12 at 0:08

3 Answers 3

up vote 2 down vote accepted

The word you are looking for is continue;

while (cin >> input)
{
    for (i = 0; i < badwords.size(); ++i)
        if (badwords[i] == input)
        {
            cout << "Bleep! ";
            continue;          // This starts the next iteration of the loop.
                               // ie we go back to the while loop and read the
                               // the next word (or attempt to do so).
        }

        cout << input << " ";
}

Also You can improve your search. Rather than using std::vector use a std::set. Then a find will automatically do a O(log(n)) search of the data.

while (cin >> input)
{
    if (badwords.find(input) != badwords.end())
    {
        cout << "Bleep! ";
        continue;
    }

    cout << input << " ";
}

Now that we have the basic algorithm we can use some of the standard algorithms:

So now replace the main loop:

std::transform(std::istream_iterator<std::string>(std::cin),
               std::istream_iterator<std::string>(),
               std::ostream_iterator<std::string>(std::cout, " "),
               BadWordFilter(badwords));

Then you just need to define the BadWordFilter

struct BadWordFilter
{
     std::set<std::string> const&  badWordSet;
     BadWordFilter(std::set<std::string> const& badWordSet) : badWordSet(badWordSet) {}

     std::string operator()(std::string const& word) const
     {
         static std::string  badWord("Bleep!");

         return (badWordSet.find(word) == badWordSet.end())
                      ? word
                      : badWord;
     }
};
share|improve this answer
    
Hm, I tried continue intially but it didn't work, I think in this case it just skipped remaining code and started the next iteration of the for loop (as i may not have reached its limit yet), whereas break would terminate all future iterations of the for loop and thus fall back into the while loop? –  njp Nov 15 '12 at 21:45
    
@DesmondWolf: The problem with your code is the variable was_bad as this was set just before the break. Then when you do a break the loop is re-started but now the next word is also ignored because you did not reset was_bad. Just drop this variable and its use. –  Loki Astari Nov 15 '12 at 21:49
    
Insightful post, lots to learn! Thankyou. –  njp Nov 15 '12 at 21:53

Instead of iterating over a vector I'd use std::find(). Even better, instead of std::vector I'd use std::set:

#include <iostream>
#include <set>
#include <string>

using namespace std;

int main(int argc, const char * argv[])
{
    string bad[] = {"poop", "balls"};
    set<string> bad_words(bad, bad + sizeof(bad) / sizeof(string));

    string input;
    while(cin >> input)
    {
        if(bad_words.find(input)!=bad_words.end()){
            cout << "bleep! ";
        }
        else {
            cout << input << " ";
        }
    }
}
share|improve this answer

I'm not sure I'd use std::set to hold the list of bad words in a case like this. std::set is (at least normally) implemented as a balanced tree, with each node separately allocated. This tends to reduce locality of reference.

By contrast, a vector is always contiguously allocated, improving locality of reference so it's more cache-friendly (and also tending to reduce heap fragmentation, for whatever that may be worth).

That doesn't mean you should forego the wonders of standard algorithms though. Quite the contrary, standard algorithms will do the job quite nicely. Since you only care about the presence/absence of a word in the "set", you can use std::binary_search to check:

std::vector<std::string> bads(std::istream_iterator<std::string>(bad_file), 
                              std::istream_iterator<std::string>);

// You can remove this sort if you're sure the words are already sorted.
std::sort(bad.begin(), bad.end());

// process the data
std::replace_copy_if(
    std::istream_iterator<std::string>(infile),
    std::istream_iterator<std::string>(),
    std::ostream_iterator<std::string>(outfile, " "),
    [](std::string const &s) { 
        return std::binary_search(bad.begin(), bad.end(), in); 
    }, 
    "bleep");

If memory serves, replace_copy_if is new with C++11, but it's fairly easy to write your own if necessary. If you don't have that, chances are pretty good you won't have lambdas available either. In this case, you should be able to replace it with a suitable invocation using std::bind1st and std::bind2nd, but I'll leave that as an exercise for the reader (personally, I'd at least consider a function object or something like boost::bind instead).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.