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

Toby Speight
87.3k14 gold badges104 silver badges322 bronze badges
asked Aug 19, 2019 at 18:13
\$\endgroup\$

1 Answer 1

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;
}
```
answered Aug 20, 2019 at 19:47
\$\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

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.