patterncppMinor
Simple Parser, C++
Viewed 0 times
parsersimplestackoverflow
Problem
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
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(co
```
#ifndef PARSER_H_
#define PARSER_H_
#include
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(co
Solution
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
As shown, I'd also remove the extraneous
I'd also change this constructor:
... to take an enumeration instead of a bool:
Alternative, I'd consider taking a pair of iterators, so the client code could pass forward iterators or reverse iterators as needed.
For:
I think I'd rather see function templates, with the predicate type passed as a template parameter:
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.
Again, I'd use the enumerated type from above instead of a Boolean (and, again, consider using iterators instead).
This assignment operator:
...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).
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 backwardI think I'd rather see function templates, with the predicate type passed as a template parameter:
template
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).
Code Snippets
parser() : begin(nullptr), end(nullptr), it(nullptr) {}parser
(
const std::string& str_to_parse,
bool start_from_the_end
):enum direction {FORWARD, REVERSE};
parser(std::string const &input, enum direction d) { // ...void pass(bool (*)(char)); //moving forward
void rpass(bool (*)(char)); //moving backwardtemplate <class F>
void pass(F f);Context
StackExchange Code Review Q#27957, answer score: 4
Revisions (0)
No revisions yet.