HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple Parser, C++

Submitted by: @import:stackexchange-codereview··
0
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

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 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 
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 backward
template <class F>
void pass(F f);

Context

StackExchange Code Review Q#27957, answer score: 4

Revisions (0)

No revisions yet.