8
\$\begingroup\$

I wrote this pretty simple program (it's an exercise from a book). It counts the words in a simple string. The book is a very old one (I think it's from 2003) and uses char arrays. Because of this, I don't know if this solution is a proper one or not.

Can you tell me whether it is okay or not?

#include <iostream>

int main()
{
    int count_words(std::string);

    std::string input_text;
    std::cout<< "Enter a text: ";
    std::getline(std::cin,input_text);

    std::cout << "Number of words: " << count_words(input_text) << std::endl;
    return 0;
}
int count_words(std::string input_text)
{
    int number_of_words = 1;
    for(int i = 0; i < input_text.length();i++)
        if(input_text[i] == ' ')
            number_of_words++;
    return number_of_words;
}
\$\endgroup\$
  • \$\begingroup\$ If you enter multiple spaces each will register as a word Try: 'A B C' (note two spaces between words). This will return 5. Its an OK algorithm to start with as long as they then proceed to teach you more. \$\endgroup\$ – Martin York Aug 31 '13 at 16:47
12
\$\begingroup\$
  • Prefer std::size_t for the word count and count_words()'s return type. Also prefer std::string::size_type for the loop counter (when using indices is needed).

  • count_words()'s parameter should be passed by const& since it's not being modified:

    std::size_t count_words(std::string const& input_text) {}
    
  • count_words() has a problem. If I press ENTER without typing anything, it gives me 1. To fix this, return 0 if the string is empty:

    if (input_text.empty()) return 0;
    
  • In the for-loop, prefer std::isspace for detecting whitespace.

  • Prefer iterators over indices for std::string:

    for (auto iter = input_text.cbegin(); iter != input_text.cend(); ++iter)
    {
        if (*iter == isspace)
        {
            number_of_words++;
        }
    }
    

    If you have C++11, use this range-based for-loop instead:

    for (auto& iter : input_text)
    {
        if (iter == isspace)
        {
            number_of_words++;
        }
    }
    
  • As @r-s mentions, this just counts whitespace. Here's another solution from this answer:

    unsigned int countWordsInString(std::string const& str)
    {
        std::stringstream stream(str);
        return std::distance(std::istream_iterator<std::string>(stream), std::istream_iterator<std::string>());
    }
    

    I realize this may be difficult to understand. I'd still recommend holding onto the answer for future reference. If you want to try it out, include <sstream> and <iterator>.

\$\endgroup\$
  • \$\begingroup\$ thanks :) I have some questions :) Which is better? Using function prototypes or putting the whole function under the main()? I should always use std::size_t when I only need positive values? Is it better then unsigned int e.g? \$\endgroup\$ – mitya221 Aug 31 '13 at 7:25
  • \$\begingroup\$ It's better to use function prototypes (above main) and use functions. OR, put main at the very bottom and you won't need the prototypes. \$\endgroup\$ – Jamal Aug 31 '13 at 7:30
  • \$\begingroup\$ Any unsigned type is preferred when only using positive numbers. So, both of them work for that. Here's a more specific definition of std::size_t: cplusplus.com/reference/cstddef/size_t. \$\endgroup\$ – Jamal Aug 31 '13 at 7:32
  • \$\begingroup\$ thanks :D actually my question was wrong (I meant above main) but I thing you understood it :) \$\endgroup\$ – mitya221 Aug 31 '13 at 7:32
  • \$\begingroup\$ You're welcome! Again, I'll continue to look into the last point I mentioned. \$\endgroup\$ – Jamal Aug 31 '13 at 7:33
6
\$\begingroup\$

I am far from an expert in C++ but I am going to give this a shot.

int count_words(std::string);

you have this declared inside of the main() function. This is a function prototype and should be outside of the main function ex:

#include <iostream>

int count_words(std::string);

int main()
{
    std::string input_text;

In the count_words function, if somehow a user entered a string with 0 words in it, the function would return 1 which would be incorrect. A solution to this is to check if the string is empty, and if it is, we can return 0. There are a few ways to do this, but I think this way makes sense here:

if (input_text.empty()) return 0;

Keep in mind that we are not really counting the number of words in the string with this function, we are simply counting the number of spaces and hoping that the number of spaces plus 1 is equal to the number of words. This will be fine for most text but will fail on example strings such as:

this  will  return  SEVEN

Just something to keep in mind.

\$\endgroup\$
  • \$\begingroup\$ Nothing wrong with forward declaring inside a function. \$\endgroup\$ – Martin York Aug 31 '13 at 16:51
4
\$\begingroup\$

A few more points:

You are using std::string, and hence should have an #include <string>. <iostream> pulls it in for you in this case, but it's bad to rely on such behaviour.

Likely this is done as a learning exercise. When you get more comfortable with C++, it's good to look into what the algorithms that are part of the C++ standard library can provide you with. In fact, std::string is a sequence, and it exposes iterators. This means that just about every standard algorithm can be used with it. In this case, there is an algorithm for counting: std::count and std::count_if. Using these with std::isspace can make your code both readable and succinct:

#include <algorithm>
#include <cctype>

std::size_t count_words(const std::string& str)
{
    return std::count_if(str.begin(), str.end(), std::isspace);
}

In fact, with the point above that any white space that sits together will be counted as an extra word, you can get quite fancy with the standard algorithms to fix this:

std::size_t count_words(const std::string& str)
{
    std::string copy_(str);
    copy_.erase(std::unique(copy_.begin(), copy_.end(), 
                [](char a, char b) 
                { return isspace(a) && isspace(b); }),
                copy_.end());
    return std::count_if(copy_.begin(), copy_.end(), std::isspace);
}

This is all probably quite mysterious for the moment, but hopefully your book will start guiding you into how to use the standard algorithms to simplify your code.

\$\endgroup\$
  • \$\begingroup\$ std::distance() and reading words seems more logical. As it reads the input by words. If you want to be exact you just need to convert punctuation to space (this can be done via local rather than modifying the input text). \$\endgroup\$ – Martin York Aug 31 '13 at 16:55
  • \$\begingroup\$ I'm not sure if I'd call it more logical - more performant, perhaps (I realise my solution does a reasonable amount of copying and 2 passes over the string). You can certainly pass a different lambda/function/functor to count_if to deal with punctuation. \$\endgroup\$ – Yuushi Sep 1 '13 at 5:44
3
\$\begingroup\$

One complication might be that if you put a whole bunch of spaces between your words then that increases the word count each time, so " microwave " is three words. Also "hello\nworld" is counted as one word. Should "phase-change" be one word or two?

How about detecting the boundaries between word and not-word?

  unsigned number_of_words = 0;
  previous_char_was_word_character = false;

  for(...)
  {
    word_character = is_word_character(input_text[i]);

    if(word_character && !previous_char_was_word_character)
    {
      number_of_words++;
    }

    previous_char_was_word_character = word_character;
  }

As to the definition of what characters count as being "in a word", that's a matter of opinion.

One way of demonstrating this is to consider what happens when you use the ctrl+← and ctrl+→ keys to move back and forth a word in different word-processors.

For example, AbiWord, and LibreOffice count previous_char_was_word_character as 9 words (stopping at the beginning and end of each underscore). GEdit and the Firefox textarea count it as 5 words. Windows' Notepad probably counts it as 1 word. If you used Perl's \w regular expression to detect, then it would count as 1 word.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good algorithm for checking if it is a word. \$\endgroup\$ – Martin York Aug 31 '13 at 17:36
3
\$\begingroup\$

The easy way to do it (in steps)

The std::istream_iterator<X> reads X from a stream using operator>>.
If X is std::string then it reads one white space separated word from the stream.

std::string        words = /* You fill it somehow (maybe getline()) */;
std::stringstream  wordstream(words);

std::size_t wordCount = 0;
for(auto loop = std::istream_iterator<std::string>(wordstream) != std::istream_iterator<std::string>(); ++loop)
{
    ++wordCount;
}

Since we are using a loop we can say lets try and use a standard algorithm. This case we are just measuring the number of items from beginning to end of the stream so the easy one to use is std::distance(). In this case the loop can be replaced like this:

std::string        words = /* You fill it somehow (maybe getline()) */;
std::stringstream  wordstream(words);

std::size_t wordCount = std::distance(std::istream_iterator<std::string>(wordstream),
                                      std::istream_iterator<std::string>());

But hold on.
The operator>> only counts white space as word separators. But if we had a real line. There may be other punctuation on the line"

I have,bad,punctuation in this line.

This would result in 5 words:

I
have,bad,punctuation
in
this
line.

So what you need to do is imbue the stream with information about how to break a word. In this case we will just turn all punctuation into white space.

#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;
            // Add more punctuation here.
        }
    private:
        base::mask  table[256];
};

Now you code becomes:

std::string        words = /* You fill it somehow (maybe getline()) */;
std::stringstream  wordstream;
wordstream.imbue(std::locale(std::locale(), new WordSplitterFacet(std::locale())));
wordstream << words;

std::size_t wordCount = std::distance(std::istream_iterator<std::string>(wordstream),
                                      std::istream_iterator<std::string>());

I would take this a step further.
I would create a class that wraps the string stream and does the imbue for you:

class RealWordStream: public std::stringstream
{
    RealWordStream(std::string const& input)
    {
        imbue(std::locale(std::locale(), new WordSplitterFacet(std::locale())));
        (*this) << words;
    }
};

Now Your code returns to the simpler looking:

std::string        words = /* You fill it somehow (maybe getline()) */;
RealWordStream     wordstream(words);

std::size_t wordCount = std::distance(std::istream_iterator<std::string>(wordstream),
                                      std::istream_iterator<std::string>());
\$\endgroup\$
  • \$\begingroup\$ This is quite a lot of boilerplate just to change characters. Is there a way to make it shorter? \$\endgroup\$ – idoby Aug 31 '13 at 17:35
  • \$\begingroup\$ @busy_wait: If you mean the WordSplitterFacet, then not that I know. But modifying the facets is a very rare operation. Write this once put it in a library ready for re-use. \$\endgroup\$ – Martin York Aug 31 '13 at 17:42
  • \$\begingroup\$ Where can I learn more about this technique/these parts of the STL? Does this modify the string itself? (I'm no STL expert) \$\endgroup\$ – idoby Aug 31 '13 at 17:45
  • 1
    \$\begingroup\$ Search Stackoverflow for imbue. No it does not change the underlying string. \$\endgroup\$ – Martin York Aug 31 '13 at 17:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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