3
\$\begingroup\$

For my computer science class, I was required to write a lexical analysis program that would perform several functions on a std::string. The assignment required a function for each of the following:

  • count number of a certain substring
  • count number of words excluding numbers
  • count number of unique words (excludes repeated words)
  • count number of sentences (by end punctuation)
  • average words per sentence
  • lexical density as percent (unique word count / word count * 100)

My code is fairly long, but here it is:

#include<iostream>
#include<string>
#include<sstream>
#include<algorithm>
#include<iterator>
#include<vector>

using namespace std;

int substringCount(const string&, const string&);
int wordCount(const string&);
int uniqueWordCount(const string&);
int sentenceCount(const string&);
double wordsPerSentence(const string&);
double lexicalDensity(const string&); // different words / total words * 100

int main() {
    string source = ("This is the source text for this program.");
    cout << "\"is\" count: " << substringCount(source," is ") << endl;
    cout << "Word count: " << wordCount(source) << endl;
    cout << "Sentence count: " << sentenceCount(source) << endl;
    cout << "Words per sentence: " << wordsPerSentence(source) << endl;
    cout << "Lexical density: " << lexicalDensity(source) << endl;
    return 0;
}

vector<int> substringIndices(const string& str, const string& sub) { // Find indices of substrings
    vector<int> indices = {};
    for (unsigned int i = 0; i < str.size(); i++) {
        unsigned int t = i;
        for (unsigned int j = 0; j < sub.size(); t++, j++) {
            if (t >= str.size()) {
                break;
            }
            if (str[t] == sub[j]) {
                if (j + 1 == sub.size()) indices.push_back(i);
                continue;
            } else {
                break;
            }
        }
    }
    return indices;
}

vector<string> splitByWhitespace(const string& str) { // split by whitespace
    vector<string> tokens;
    istringstream iss(str); // create istringstream
    copy(istream_iterator<string>(iss),istream_iterator<string>(),back_inserter(tokens)); // copy into tokens
    return tokens;
}

vector<string> deleteNumbers(const vector<string>& data) {
    vector<string> tr = {};
    for (unsigned int i = 0; i < data.size(); i++) {
        for (char c: data[i]) {
            if (string("0123456789").find(c) == -1) continue;
            else goto mainLoop;
        }
        tr.push_back(data[i]);
        mainLoop: continue;
    }
    return tr;
}

int substringCount(const string& str, const string& sub) { // get number of substrings
    return substringIndices(str,sub).size();
}

int wordCount(const string& str) { // wordcount (based on whitespace)
    return deleteNumbers(splitByWhitespace(str)).size();
}

int uniqueWordCount(const string& str) { // unique word count
    vector<string> words = deleteNumbers(splitByWhitespace(str));
    sort(words.begin(),words.end()); // sort
    words.erase(unique(words.begin(),words.end()),words.end()); // delete extra non-unique words
    return words.size();
}

int sentenceCount(const string& str) { // get number of sentences (based on end punctuation)
    return substringCount(str,".") + substringCount(str,"?") + substringCount(str,"!");
}

double wordsPerSentence(const string& str) {
    return double(wordCount(str)) / double(sentenceCount(str));
}

double lexicalDensity(const string& str) {
    return double(uniqueWordCount(str) * 100) / double(wordCount(str));
}

I am relatively new to C++, so if there are any common practices that I am missing it would be great to point them out.

\$\endgroup\$
5
\$\begingroup\$

Name Spaces
A most common practice is to preface types and functions supplied by the Standard Template Library, sometimes known as the Standard Library with std:: rather than ignoring names spaces by the statement

using namespace std;

This will become quite helpful as the problems and code become more complex and you need to include other name spaces. Some languages such as C don't support namespaces, however, C++ does and it is quite a useful feature. More complex programs may use multiple name spaces and each one of these name spaces can contain a definition for functions such as sort() or find_if(), or overrides on operators such as <<. You may even need to write your own sort() or find(). Using std::sort allows you not to create your own name space when you write your own sort() with the same arguments.

Initialization of Empty Containers
There is no need for the empty braces on this line:

    vector<int> indices = {};

The vector container class has a constructor that will initialize an empty vector properly.

    vector<int> indices;

Use the Features of the Container Classes and the Standard Template Library
You may find this website useful for learning all of the features a particular container class or the standard library.

There are definitely some functions you could be using, such as std::find(), std::find_if(), std::count_if() and std::string::substr() that could definitely shorten the code.

It might be wise to investigate std::map as well for counting words.

GOTO
There are almost always ways to avoid using goto in C++. In rare cases goto may be appropriate for error handling, more so in C than in C++ because C++ has try{}/catch{} and exceptions.

In the following code there is really no reason to use a goto:

vector<string> deleteNumbers(const vector<string>& data) {
    vector<string> tr = {};
    for (unsigned int i = 0; i < data.size(); i++) {
        for (char c: data[i]) {
            if (string("0123456789").find(c) == -1) continue;
            else goto mainLoop;
        }
        tr.push_back(data[i]);
        mainLoop: continue;
    }
    return tr;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ On the GOTO thing: breaking out of nested loops is actually another one of those rare legitimate use cases for it. I think the GOTO could be valid for this reason. If the nested for loop is legitimate or not is a different question. If it it is unnecessary, so becomes the GOTO. \$\endgroup\$ – I'll add comments tomorrow Sep 20 '16 at 7:46
2
\$\begingroup\$

White space

Though this is technically correct.

#include<iostream>
#include<string>
#include<sstream>
#include<algorithm>
#include<iterator>
#include<vector>

It looks very strange without the space after the include. Remember that you are writing the code for humans to read so try and make it easy for them to read. The extra horizontal space will give that visible break that makes it easy to read.

Namespace

Avoid the using statement.

using namespace std;

I know all the books use it (they are trying to save space for printing and thus costs). But in real programs over 10 lines long this starts to become an issues. But even in programs under 10 lines this should be avoided to maintain good habits.

As a side note. The reason the "standard" library is in the namespace "std" and not "standard" is so that prefix each type/object with "std::" is not excessively cumbersome.

For details about the issues read: Why is “using namespace std” considered bad practice?

Prefer prefix increment over suffix increment.

        for (unsigned int j = 0; j < sub.size(); t++, j++) {

OK; for integer arithmetic this makes absolutely no difference. But this common pattern is repeated for iterators and other types. In these situations the default implementation of prefix and suffix make the prefix version more efficient. see: How to overload the operator++ in two different ways for postfix a++ and prefix ++a?

But you should still prefer the prefix version because a maintainer may come along and change the types being used. If this happens and you are already using the prefix version then no other changes will be needed in the code (as you already have the most efficient version).

Standard algorithms

Try and learn the standard algorithms. That way you are not re-inventing the wheel when writing the code.

For substringIndices() you may want to look at std::string::find(const string& str, size_t pos = 0).

Note: std::vector<> can be constructed using iterators. So in splitByWhitespace() you don't need to use std::copy().

    vector<string> tokens;
    copy(istream_iterator<string>(iss),istream_iterator<string>(),back_inserter(tokens)); // copy into tokens

    // Can be simplified too: (c++11)
    vector<string> tokens(istream_iterator<string>{iss},istream_iterator<string>{});

    // Can be simplified too: (c++03)
    vector<string> tokens(istream_iterator<string>(iss),(istream_iterator<string>()));

For deleteNumbers() you should use the Erase Remove Idiom. This will allow you to use std::string::erase() and std::remove_if()

\$\endgroup\$
  • \$\begingroup\$ I get a weird error when returning tokens without back_inserter: no viable conversion from returned value of type 'std::vector<std::string> (std::istream_iterator<std::string>, std::istream_iterator<std::string> (*)())' (aka 'vector<basic_string<char, char_traits<char>, allocator<char> > > (istream_iterator<basic_string<char, char_traits<char>, allocator<char> > >, istream_iterator<basic_string<char, char_traits<char>, allocator<char> > > (*)())') to function return type 'std::vector<std::string>' (aka 'vector<basic_string<char, char_traits<char>, allocator<char> > >') \$\endgroup\$ – AMACB Sep 20 '16 at 16:43
  • \$\begingroup\$ Sorry. That's caused by Most vexing parse I have updated my answer with the correct syntax to void it (for C++11). If you are still using C++03 it's slightly less obvious. But you can wrap the second iterator with braces. (istream_iterator<string>()) \$\endgroup\$ – Martin York Sep 20 '16 at 16:51
  • \$\begingroup\$ I love these names for errors :). Thanks for the feedback! \$\endgroup\$ – AMACB Sep 20 '16 at 20:19

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.