How effective is my word parser?

I took about 30 minutes to work out the kinks in my parser, but I would like some feedback or any criticism/comments/compliments on my code thus far!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
#ifndef CCOMMANDPARSER_H
#define CCOMMANDPARSER_H
#include <iostream>

class Ccommandparser
{
	public:
		void getCommand();
		void findWords();
	private:
		const static unsigned int LIMIT = 10;
		unsigned int startPos, endPos, wordsFound;
		std::string sentence;
		std::string word[LIMIT];

};

#endif 


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
#include "ccommandparser.h"
#include <iostream>

void Ccommandparser::getCommand()
{
	std::cout << "Please enter your decision." << std::endl;
	getline(std::cin, sentence);	
	findWords();
}

void Ccommandparser::findWords()
{
	startPos = 0; 
	endPos = 0;
	wordsFound = 0;
	
	for ( unsigned int current = 0; current <= sentence.length(); current++ ) 
	{
		endPos = current;
		
		if ( sentence[current] == ' ' || current == sentence.length() ) // Iterate through sentence checking for empty space OR end of string
		{
			for ( unsigned int copying = startPos; copying < endPos; copying++ ) // Copy chars from string from start position to end position
			{
				word[wordsFound] += sentence[copying];
			}
			wordsFound++; // add to words found, queing up next array location for next word
			startPos = endPos + 1; // New starting position is one PAST the empty space that was located
		}
		if ( wordsFound == LIMIT ) // if more than LIMIT words are found, exit function
		{
			break;
		}
	}
}
Last edited on
Not had time to look at the logic as such but..
is there ever a chance that "wordsFound" variable will be greater than 10 (i.e. the value of LIMIT)?

Also i'd consider using an stl container like std::vector over a horrible c-style array any day.

Also if startPos, endPos, and wordsFound are only going to be used in findWords() then define them as local variables. Similarly for sentance in getCommand(). (although i can see they would be useful member variables if you are going to expand this class).
Last edited on
I'd do something like this:
1
2
3
4
5
6
7
8
9
10
11
12
void Tokenize(const std::string s, std::vector<std::string>& v)
{
  std::istringstream iss(s);
  while (true)
  {
    std::string str;
    iss >> str;
    if (iss.eof()) 
      return; 
    v.push_back(str);
  }
}


or
1
2
3
4
5
void Tokenize(const std::string s, std::vector<std::string>& v)
{
  std::istringstream iss(s);
  std::copy( std::istream_iterator<std::string>(iss), std::istream_iterator<std::string>(), std::back_inserter(v) );
}
Last edited on

Not had time to look at the logic as such but..
is there ever a chance that "wordsFound" variable will be greater than 10 (i.e. the value of LIMIT)?
1
2
3
4
		if ( wordsFound == LIMIT ) // if more than LIMIT words are found, exit function
		{
			break;
		}

Since it's a loop that iterates 1 value at a time, it will never actually go past 10. I've tested it out, if theres 11, 12, or 100 words, it only records first 10.

I made them local variables now, and i pass them by reference to save memory.
Last edited on
Topic archived. No new replies allowed.