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...
1 Answer 1
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;
}
```
-
\$\begingroup\$ Is your is_one_of template right? ideone.com/PZ5kT1 \$\endgroup\$noman pouigt– noman pouigt2019年08月23日 16:45:58 +00:00Commented 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\$G. Sliepen– G. Sliepen2019年08月23日 17:42:04 +00:00Commented Aug 23, 2019 at 17:42