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.

I have written this program to count the number of words in a string. I have checked my program for the worst-case scenario. If any of you can find any cases for which this program doesn't work, please let me know so that I can work and improve it.

And just to be clear, we don't want symbols like, "," , "!" , "?", "." , "\n" to be counted as words. But obviously, "I" should be counted as word, as we consider it in the language. I have made sure of all this by replacing them with spaces. Let me know if I have missed something.

Is this a good solution? How can I improve the code?

#include <iostream>
#include <string> 

void replace(std::string& str, char x, char y);
int countWords(std::string x);

int main(){
std::cout<<countWords(" \n \t Hello, world ! ");
}

void replace(std::string& const str, char x, char y){
for(unsigned int i=0;i<str.size();i++){
    if(str[i]==x) str[i]=y;
}
}

int countWords(std::string x){

replace(x,',',' ');
replace(x,'.',' ');
replace(x,'!',' ');
replace(x,'?',' ');
replace(x,'(',' ');
replace(x,')',' ');
replace(x,'\n',' ');
replace(x,'\t',' ');
replace(x,'"',' ');

if(x.empty()) return 0;
int Num=1;

for(unsigned int i=1;i<x.size();i++){
    if(x[i]==' ' && x[i-1]!=' ') Num++;
}
if(x.back() == ' ') Num--;
return Num;
}
share|improve this question
    
Was there a problem copying this from your IDE? There are many inconsistencies in indentation here. –  Jamal Jul 19 at 17:22
    
Sorry for that. I am new here, so actually the problem was in pasting the code. –  user3834119 Jul 19 at 17:25
    
That's fine. Are you going for a purely algorithmic way, or just the most idiomatic way (utilizing the STL)? –  Jamal Jul 19 at 17:33
1  
A small note: consider using std::size_t for counting instead of int -- it expresses the intent better (non-negative size -- as opposed to an arbitrary / possibly negative integer), it is more established/idiomatic (both C and C++ std libs use it for counts), and there's more of a guarantee that it's large enough to store sizes (int is only guaranteed to represent 16 bits, which for a signed type means values up to 32767: en.cppreference.com/w/cpp/language/types#Integer_types). –  Matt Jul 19 at 19:26

4 Answers 4

up vote 2 down vote accepted

An alternative to my first version.

If you don't want to use streams. Then I would go with a version that does not modify the original string.

int wordCount(std::string const& words)
{
    int    count    = 0;
    bool   inWord   = false;

    foreach(char loop: words)
    {
         /*
          * If we are not in a word and hit a valid letter (part of word)
          * Then increment the word count and mark we are in the middle of
          * a word (so now the else part of this is active for subsequent
          * iterations.
          */
         if (!inWord) {
             if (isLetter(loop)) {
                 ++count;
                 inWord = true;
             }
         } else {
         /*
          * If we are in a word and hit an invalid letter (not part of word)
          * Then mark that we have finished a word. This allows the
          * the main part of the if to be enetered on the next iteration.
          */
             if (!isLetter(loop)) {
                 inWord = false;
             }
         }
    }
    return count;
}

So now you need to define isLetter() efficiently so it only applies to characters that appear in words.

    bool isLetter(char v)
    {
        /*
         * Return true for any character that is legally part of a word.
         * currently this is a-z and A-Z but the question was unclear as
         * to the extent of what counts as part of the word so this array
         * can be adjusted as appropriate to include valid characters.
         *
         * Note: If this is really just a-z and A-Z then it can be replaced
         *       by a single call to std::isalpha() which does exactly
         *       the same thing.
         *
         * Note II: The use of the array is to trade space for time.
         *          This will be faster. But it also assumes we are using
         *          the ASCII character set.
         *
         *          The use of the array can be traded for another technique
         *          this function is simply meant to define the letters that
         *          are valid parts of a word and any technique could be used.
         */
        static bool letter[] =
        {
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           // A-Z starts at 65
           0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
           1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0,
           // a-z starts at 97
           0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
           1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
           // Modify as appropriate.
        };
        return letter[static_cast<unsigned char>(v)];
    }
share|improve this answer
    
Yes, i didn't want to use the streams. This answer is good, but could you please explain the logic a little bit? Then it would be easy for me(beginner) to understand. –  user3834119 Jul 20 at 7:26
    
Added comments. But in my opinion they should not be needed (even by a beginner). The code was self documenting and adding the comments makes the code worse as they now need to be maintained. –  Loki Astari Jul 20 at 17:30
1  
PS You should learn how to use stream version. It allows for much more sophisticated techniques later. –  Loki Astari Jul 20 at 17:30

I personally would use the built-in way of reading words.

std::stringstream wordStream("A list of words");

std::string       word;
int               count = 0;
while(wordStream >> word)
{
    ++count;   // We read one space separated words
}              // from the stream.

Of course the default does not handle punctuation correctly (as the words are divided by space. But we can handle that.

So how do we make things look like a space.
All you need to do is tell the stream what characters are spaces. To do this you need to create a ctype facet and implant it into the stream.

#include <locale>
#include <string>
#include <sstream>
#include <iostream>

// This is my facet that will treat the ,.- as space characters and thus ignore them.
class WordSplitterFacet: public std::ctype<char>
{
    public:
        typedef std::ctype<char>    base;
        typedef base::char_type     char_type;

        WordSplitterFacet(std::locale const& l)
            : base(table)
        {
            std::ctype<char> const&  defaultCType  = std::use_facet<std::ctype<char> >(l);

            // Copy the default value from the provided locale
            static  char data[256];
            for(int loop = 0;loop < 256;++loop) { data[loop] = loop;}
            defaultCType.is(data, data+256, table);

            // Modifications to default to include extra space types.
            table[',']  |= base::space;
            table['.']  |= base::space;
            table['!']  |= base::space;
            table['?']  |= base::space;
            table['(']  |= base::space;
            table[')']  |= base::space;
        }
    private:
        base::mask  table[256];
};

Now you just have to add it to a stream.

int main()
{
    std::stringstream   wordsAndPunt("word1 ,,, word2");
    wordsAndPunt.imbue(std::locale(std::locale(), new WordSplitterFacet(std::locale()));

    // Now apply the count.
    std::string word;
    int         count = 0;
    while(wordsAndPunt >> word)
    {
        ++count;   // We read one space separated words
    }              // from the stream.
    std::cout << count << "\n";
}

Now we have the basics in place you don't even need the loop. You can replace it with standard algorithms.

int main()
{
    std::stringstream   wordsAndPunt("word1 ,,, word2");
    wordsAndPunt.imbue(std::locale(std::locale(), new WordSplitterFacet(std::locale()));

    // Now apply the count.
    int  count = std::distance(std::istream_iterator<std::string>(wordsAndPunt),
                               std::istream_iterator<std::string>());
    std::cout << count << "\n";

}
share|improve this answer

One simple way is to use the ispunct() and isspace() functions on each character. A simple boolean keeps track of whether you have consecutive punctuation/space:

size_t CountWords(string input)
{
    //A string with no punctuation/space is one word.
    size_t output = 1;
    char tempc = *input.rbegin();
    if (ispunct(tempc) || isspace(tempc))
        output = 0;
    bool consecutive = false;
    for (char c : input)
    {
        if ( (ispunct(c) || isspace(c)))
        {
        if (!consecutive)
            output++;
        consecutive = true;
    }
    else

        consecutive = false;
    }
    return output;
}

If a custom delimiter list is required something like this would work:

size_t CountWords(string input, string delimiters)
{
    size_t output = 1;
    string::const_iterator index = delimiters.cbegin();
    string::const_iterator end = delimiters.cend();
    if (find(index, end, *input.rbegin()) != end)
        output = 0;
    bool consecutive = false;
    for (char c : input)
    {
        if (find(index, end, c) != end)
        {
        if (!consecutive)
            output++;
        consecutive = true;
    }
    else
        consecutive = false;
    }
    return output;
}
share|improve this answer

It's simple really, just first eat the white-space, then do the counting, it can't get more simple than this

#include <assert.h>
#include <string> 
#include <cctype> // std::isspace

size_t count_words(const std::string& s){
    // eat whitespace
    typedef std::string::const_iterator SI;
    SI iter(s.begin()),end(s.end());
    for(;iter!=end;++iter){
        if(std::isspace(*iter) == 0)
            break;
    }
    // either at the end or at the first non-whitespace
    assert( iter == end  || std::isspace(*iter) == 0);

    size_t words(0);
    for(;iter!=end;){
        // always at the start of a word, never at the end
        assert(iter!=end && std::isspace(*iter) == 0); 
        do{
            // if is space
            if(std::isspace(*iter) != 0)
                break;
            assert( iter == end || std::isspace(*iter) == 0);
            ++iter;
        }while(iter!=end);
        // either at the end or at the first whitespace after the current word
        assert( iter == end || std::isspace(*iter) != 0 );
        for(;iter!=end;++iter){
            if(std::isspace(*iter) == 0)
                break;
        }
        // either at the end or at the begging of the next word
        assert( iter == end || std::isspace(*iter) == 0 );
        ++words;
    }
    return words;   
}
share|improve this answer
1  
Could you please explain the alterations you made and the decisions behind them? That would improve your answer immensely! I'd like to know what makes you think "it can't get more simple than this". –  Alex L Jul 20 at 5:23
1  
I think both my versions are simpler than this. You have two different loops for eating white space why not combine them into one. One of the things the OP pointed out was punctuation, but you have not taken that into account. –  Loki Astari Jul 20 at 17:33
    
If all you want to do is count white space separated words that's a one line operations. return std::distance(std::istream_iterator<std::string>(input), std::istream_iterator<std::string>()); –  Loki Astari Jul 20 at 17:36

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.