5
\$\begingroup\$

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);
}
\$\endgroup\$
3
  • \$\begingroup\$ I don't understand what you are trying to achieve? \$\endgroup\$ Jul 2 '13 at 6:10
  • \$\begingroup\$ I want write a class which offers an interface to working with strings without using of iterators or [] operators. What is wrong with this code and how to do it better? \$\endgroup\$
    – user21974
    Jul 2 '13 at 14:16
  • \$\begingroup\$ OK. You are aware of the std::stream interface. You can use it via strings with the class std::stringstream. \$\endgroup\$ Jul 2 '13 at 15:33
4
\$\begingroup\$

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).

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy