I wrote a simple parser for input-output operators. What is wrong with this code and how could it be better?
#ifndef PARSER_H_
#define PARSER_H_
#include <string>
class parser {
public:
typedef std::string::const_iterator const_iterator;
private:
static const_iterator start_position(bool b, const std::string& str);
static const_iterator begin_it(const parser& p);
static const_iterator end_it(const parser& p);
static const_iterator current_it(const parser& p);
const_iterator begin;
const_iterator end;
const_iterator it;
public:
parser(): begin(NULL), end(NULL), it(NULL) {};
parser
(
const std::string& str_to_parse,
bool start_from_the_end
):
begin(str_to_parse.begin()),
end (str_to_parse.end()),
it (start_position(start_from_the_end, str_to_parse)) {};
//Give another string to parser:
void str(const std::string&);
void set_to_begin();
void set_to_end();
parser& operator = (const parser&);
bool eof_str() const;
char get(); //move forward
char rget(); //move backward
char peek() const; //watch current symbol
//pass an all symbols beginning from current:
void pass(char); //moving forward
void rpass(char); //moving backward
//pass an all symbols beginning from
//current which satisfy to condition:
void pass(bool (*)(char)); //moving forward
void rpass(bool (*)(char)); //moving backward
//return iterator:
const_iterator current_it() const;
};
//This function is used in constructor:
//it helps to set iterator of parser
//to beginning or to the end of string.
inline
parser::const_iterator parser::start_position(bool b, const std::string& str) {
if (b)
return str.end();
return str.begin();
}
//This functions are used in operator=.
//I decided to do not writing analogous
//const-functions for better encapsulation.
inline
parser::const_iterator parser::begin_it(const parser& p) {return p.begin;}
inline
parser::const_iterator parser::end_it(const parser& p) {return p.end;}
inline
parser::const_iterator parser::current_it(const parser& p) {return p.it;}
inline
void parser::str(const std::string& str_to_parse) {
begin = str_to_parse.begin();
end = str_to_parse.end();
it = str_to_parse.begin();
}
inline
void parser::set_to_begin() {it = begin;}
inline
void parser::set_to_end() {it = end;}
inline
parser& parser::operator = (const parser& p) {
begin = begin_it(p);
end = end_it(p);
it = current_it(p);
return *this;
}
inline
bool parser::eof_str() const {return it >= end;}
inline
char parser::get() {return *(it++);}
inline
char parser::rget() {return *(it--);}
inline
char parser::peek() const {return *it;}
inline
void parser::pass(char chr) {
while (*it == chr) ++it;
}
inline
void parser::rpass(char chr) {
while (*it == chr) --it;
}
inline
void parser::pass(bool (*cond)(char)) {
while (cond(*it)) ++it;
}
inline
void parser::rpass(bool (*cond)(char)) {
while (cond(*it)) --it;
}
inline
parser::const_iterator parser::current_it() const {return it;}
#endif /* PARSER_H_ */
Example:
#include "parser.h"
#include <string>
bool digit(char chr) {
return (chr >= '0' && chr <= '9');
}
std::string cut_number(parser& p, bool& error) {
error = false;
//Check on that the first
//symbol is a digit:
if (!digit(p.peek())) {
error = true;
return "";
}
parser::const_iterator begin = p.current_it();
//Check on that it is
//correct number:
if (p.get() == '0') {
if (digit(p.peek())) {
error = true;
return "";
}
}
p.pass(digit);
//In the code below could not be
//if (p.get() == '.')
//because next char after *p.it
//must be checked only if *p.it == '.'
if (p.peek() == '.') {
p.get();
//Check on that it is
//correct float pointing number:
if (!digit(p.peek())) {
error = true;
return "";
}
else p.pass(digit);
}
parser::const_iterator end = p.current_it();
return std::string(begin, end);
}
2 Answers 2
I see a few things I'd change (not really errors, but still open to improvement, IMO).
First, I'd change the default ctor to use nulltptr instead of NULL:
parser() : begin(nullptr), end(nullptr), it(nullptr) {}
As shown, I'd also remove the extraneous ; from the end of each definition, as above.
I'd also change this constructor:
parser
(
const std::string& str_to_parse,
bool start_from_the_end
):
... to take an enumeration instead of a bool:
enum direction {FORWARD, REVERSE};
parser(std::string const &input, enum direction d) { // ...
Alternative, I'd consider taking a pair of iterators, so the client code could pass forward iterators or reverse iterators as needed.
For:
void pass(bool (*)(char)); //moving forward
void rpass(bool (*)(char)); //moving backward
I think I'd rather see function templates, with the predicate type passed as a template parameter:
template <class F>
void pass(F f);
With this, the user could pass a pointer to a function as is currently allowed, but could also pass a function object instead. The possible shortcoming is that passing an incorrect parameter type might easily produce a less readable error message.
inline
parser::const_iterator parser::start_position(bool b, const std::string& str) {
if (b)
return str.end();
return str.begin();
}
Again, I'd use the enumerated type from above instead of a Boolean (and, again, consider using iterators instead).
This assignment operator:
inline
parser& parser::operator = (const parser& p) {
begin = begin_it(p);
end = end_it(p);
it = current_it(p);
return *this;
}
...looks like it's only doing what the compiler-generated operator would do if you didn't write one (in which case, I'd generally prefer to let the compiler do the job).
It seems fine.
Not sure the interface buys you anything over a normal iterator.
The only error I see is:
inline
bool parser::eof_str() const {return it >= end;}
if it> end then technically the comparison is not valid. You may want to add code to protect yourself from moving beyond the end of the string.
std::streaminterface. You can use it via strings with the classstd::stringstream. \$\endgroup\$