2
\$\begingroup\$

I have implemented Token and FieldToken classes as part of my project and would like to hear some suggestions for improvement.

First, Token class represents logical operators, relational operators, parentheses, etc. It does not hold a value, only type of the token (e.g. AND, OR, etc.). Here is its implementation:

Token.h

class Token {
public:
    enum class Type : uint8_t {
        UNKNOWN = 0,
        FIELD   = 1,

        // Logical operators
        AND = 2,
        OR  = 3,

        // Relational operators
        NEQ = 4,
        GT  = 5,
        LT  = 6,

        // Parentheses
        LP = 7,
        RP = 8
    };

public:
    Token() noexcept;
    Token(Token&& other) = default;
    Token(Token const& other) = default;
    Token(Type const type) noexcept;

    Token& operator=(Token&& other) = default;
    Token& operator=(Token const& other) = default;
    bool operator==(Token const& other) const noexcept;

    virtual ~Token() = default;

    virtual void type(Type const type) noexcept;
    Type type() const noexcept;

    bool is(Type const type) const noexcept;
    bool is_not(Type const type) const noexcept;
    bool is_one_of(Type const type1, Type const type2) const noexcept;

    template <typename... Types>
    bool is_one_of(Type const type1, Type const type2, Types const ... types) const noexcept;

    static std::unordered_map<std::string, Type> type_expressions() noexcept;

private:
    Type type_;
};

template <typename... Types>
bool Token::is_one_of(Type const type1, Type const type2, Types const ... types) const noexcept {
    return is(type1) || is_one_of(type2, types...);
}

Token.cpp

Token::Token() noexcept
    : type_(Type::UNKNOWN)
{}

Token::Token(Type const type) noexcept
    : type_(type)
{}

bool Token::operator==(Token const& other) const noexcept {
    return type_ == other.type_;
}

void Token::type(Type const type) noexcept {
    type_ = type;
}

Token::Type Token::type() const noexcept {
    return type_;
}

bool Token::is(Type const type) const noexcept {
    return type_ == type;
}

bool Token::is_not(Type const type) const noexcept {
    return type_ != type;
}

bool Token::is_one_of(Type const type1, Type const type2) const noexcept {
    return is(type1) || is(type2);
}

std::unordered_map<std::string, Token::Type> Token::type_expressions() noexcept {
    static std::unordered_map<std::string, Type> type_expressions = {
        { "and",     Type::AND },
        { "&&" ,     Type::AND },
        { "or" ,     Type::OR },
        { "||" ,     Type::OR },
        { "neq",     Type::NEQ },
        { "!="  ,    Type::NEQ },
        { "greater", Type::GT },
        { ">",       Type::GT },
        { "less",    Type::LT },
        { "<",       Type::LT },
        { "(",       Type::LP },
        { ")",       Type::RP }
    };
    return type_expressions;
}

Now, FieldToken class represents all other tokens, usually strings whose value I need to know. Therefore, I derived Token class to preserve all the functionality and added field value as a new data member.

FieldToken.h

template <typename FieldType = std::string>
class FieldToken : public Token {
public:
    FieldToken() noexcept;
    FieldToken(FieldToken&& other) = default;
    FieldToken(FieldToken const& other) = default;
    FieldToken(FieldType const& field) noexcept;

    FieldToken& operator=(FieldToken&& other) = default;
    FieldToken& operator=(FieldToken const& other) = default;
    bool operator==(FieldToken const& other) const noexcept;

    virtual ~FieldToken() = default;

    void type(Type const type) noexcept override;

    void field(FieldType const& field) noexcept;
    FieldType const& field() const noexcept;

private:
    template <typename T>
    void ignore(T&&);

private:
    FieldType field_;
};

template <typename FieldType>
FieldToken<FieldType>::FieldToken() noexcept
    : Token(Token::Type::FIELD),
      field_{}
{}

template <typename FieldType>
FieldToken<FieldType>::FieldToken(FieldType const& field) noexcept
    : Token(Token::Type::FIELD),
      field_(field)
{}

template <typename FieldType>
void FieldToken<FieldType>::type(Type const type) noexcept {
    ignore(type);
    Token::type(Token::Type::FIELD);
}

template <typename FieldType>
void FieldToken<FieldType>::field(FieldType const& field) noexcept {
    field_ = field;
}

template <typename FieldType>
FieldType const& FieldToken<FieldType>::field() const noexcept {
    return field_;
}

template <typename FieldType>
template <typename T>
void FieldToken<FieldType>::ignore(T&&)
{}

What do you think? Any implementation or logical suggestions are welcomed...

\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

Make type_expressions a const member variable

Why have a function returning a std::unordered_map<> with constant data, if you can just as well declare this map directly as a member variable? Declare this in the header file:

class Token {
    ...
public:
    static const std::unordered_map<std::string, Type> type_expressions;
    ...
};

And the following in the implementation file:

const std::unordered_map<std::string, Type> Token::type_expressions = {
    {"and", Type::AND},
    ...
};

Consider having Token just be a class enum

Your class Token is not very generic. It works like an enum, with just two differences: it has a is_one_of() convenience function, and there's a map from strings to Type. But it has a hardcoded set of Types. I recommend just doing:

enum class Token: uint8_t {
    UNKNOWN,
    FIELD,
    ...
};

Have the map from strings to types just be a non-class variable, and defining is_one_of() as a generic function that can compare any set of things:

template<typename T, typename... Ts>
bool is_one_of(T const value, T const value1, Ts const ... values) {
    return value == value1 || is_one_of(value, values...);
}

template<typename T>
bool is_one_of(T const value, T const value1) {
    return value == value1;
}
```
\$\endgroup\$
2
  • \$\begingroup\$ Is your is_one_of template right? ideone.com/PZ5kT1 \$\endgroup\$ Commented Aug 23, 2019 at 16:45
  • \$\begingroup\$ @nomanpouigt No it's not, I always forget you can't have a variable number of arguments with the same type this way. \$\endgroup\$ Commented Aug 23, 2019 at 17:42

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.