I am relatively new to C++ coming from languages such as Java and would like tips to make my code more C++ like and doing things in the C++ way. The header file:
#pragma once #include #include class Expression < public: std::string symbol; std::shared_ptrleft; std::shared_ptr right; float eval(); static Expression parse(std::string s); private: Expression(std::string symbol, Expression *left, Expression *right); static Expression parseRec(std::string s); static float evalRec(const Expression &e); >; #include "Expression.hpp" #include Expression::Expression(std::string symbol, Expression *left, Expression *right) : symbol(symbol), left(left), right(right) <> float Expression::eval() < return Expression::evalRec(*this); >float Expression::evalRec(const Expression &e) < switch (e.symbol[0]) < case '+': return evalRec(*e.left) + evalRec(*e.right); case '-': return evalRec(*e.left) - evalRec(*e.right); case '*': return evalRec(*e.left) * evalRec(*e.right); case '/': return evalRec(*e.left) / evalRec(*e.right); default: return std::stoi(e.symbol); >> Expression Expression::parse(std::string s) < //Remove whitespace std::string output; for (auto &i : s) < if (i != ' ') output += i; >return parseRec(output); > Expression Expression::parseRec(std::string s) < const std::unordered_mapprecedence = , , , >; int indexOfLowest = -1; int i = 0; for (auto &&j : s) < switch (j) < case '+': case '-': case '*': case '/': if (indexOfLowest == -1) indexOfLowest = i; else if (precedence.at(j) i++; > if (indexOfLowest == -1) < return Expression(s, NULL, NULL); >else < return Expression(std::string(1, s[indexOfLowest]), new Expression(parseRec(s.substr(0, indexOfLowest))), new Expression(parseRec(s.substr(indexOfLowest + 1, s.length() - indexOfLowest - 1)))); >> #include #include "Expression.hpp" int main(int argc, char const *argv[]) < //Parsed expression tree Expression e = Expression::parse("5 + 10 * 2 + 6 / 3"); std::cout
78.7k 14 14 gold badges 95 95 silver badges 278 278 bronze badges
asked Jan 18, 2021 at 11:51
Thomas Briggs Thomas Briggs
103 4 4 bronze badges
\$\begingroup\$ Welcome to Code Review! Thanks for this great question - I hope you get some good reviews, and I hope to see more of your contributions here in future! \$\endgroup\$
Commented Jan 18, 2021 at 13:02Here are some things that may help you improve your code.
There should be an include guard in each .h file. That is, start the file with:
#ifndef EXPRESSION_H #define EXPRESSION_H // file contents go here #endif // EXPRESSION_H The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8
C++ provides a great number of useful utilites such as std::remove that you could use instead of your own hand-written function to remove whitespace. They're almost always faster and less buggy and by using library software, you can develop your own code more quickly and accurately. So instead of this:
//Remove whitespace std::string output; for (auto &i : s) < if (i != ' ') output += i; >return parseRec(output); One could use this:
s.erase(std::remove(s.begin(), s.end(), ' '), s.end()); return parseRec(s); Note that this uses s rather than output per the next suggestion.
One thing that is quite different about C++ as compared with Java is how parameter passing is done. In general, there is pass-by-value and pass-by-reference. The pass-by-value is what we get when we write things like this:
static Expression parse(std::string s); The gives parse its own copy of s . If we use pass-by-reference, it would generally look something like this:
static Expression parse(const std::string& s); Now we are giving parse a reference (indicated by & ) rather than copying it. In this example, we use the very common idiom of passing a const reference. This indicates that the called function parse may examine but not alter the value. Unlike Java, there is no automatic garbage collector in C++, so it's up to you to manage memory. Generally, passing by reference is faster and uses less memory, but in cases where we would be making a copy anyway, passing by value makes sense.
The Expression class has its only data members as public members. Rather than do it that way, it would be better to keep them private. See C.9 for details.
When a non-static member function is called in a C++ class, *this is implicitly passed. So the result is that this function is entirely useless:
float Expression::eval()
Instead, just rename evalRec as eval and write it as a member function:
float Expression::eval() < switch (symbol[0]) < case '+': return left->eval() + right->eval(); case '-': return left->eval() - right->eval(); case '*': return left->eval() * right->eval(); case '/': return left->eval() / right->eval(); default: return std::stoi(symbol); > > The parseRec routine uses a std::ordered_map and also a switch to find the operator with the lowest precedence. I suggest that when you see that you have repeated the operators in multiple places, this is a hint that perhaps you could use a different approach to avoid this duplication. In this case, my inclination would be to use std::min_element . This requires beginning and ending iterators and a comparison operation. First, let's write a comparison operation:
enum class op_precedence < LOW, MED, NON_OP >; static constexpr op_precedence precedence(char ch) < op_precedence val; switch (ch) < case '+': case '-': val = op_precedence::LOW; break; case '*': case '/': val = op_precedence::MED; break; default: break; >return val; > Now we can use std::min_element :
std::string::iterator lowest) >; This uses a lambda to actually effect the comparison. So now the whole function looks like this:
Expression Expression::parseRec(std::string s) < std::string::iterator lowest) if (precedence(*lowest) == op_precedence::NON_OP) < return Expression(s, nullptr, nullptr); >else < return Expression(std::string(1, *lowest), new Expression(parseRec(s.substr(0, lowest-s.begin()))), new Expression(parseRec(s.substr(lowest-s.begin() + 1))) ); >> Note, too, that this uses the modern nullptr rather than the old C-style NULL .
I know you're just beginning with C++ (and it looks like a promising beginning!) but to whet your appetite for more, we can actually go a little further with the concept mentioned above. Specifically, we have a similar duplication in eval , so we could further consolidate information. Here's one way to do it:
enum class op_precedence < LOW, MED, NON_OP >; struct op < op_precedence precedence; float (*operation)(float, float); >; static const std::unordered_map operation = < < '+', < op_precedence::LOW, [](float a, float b)> >, < '-', < op_precedence::LOW, [](float a, float b)> >, < '*', < op_precedence::MED, [](float a, float b)> >, < '/', < op_precedence::MED, [](float a, float b)> >, >; static op_precedence precedence(char ch) < auto myop; return myop == operation.end() ? op_precedence::NON_OP : myop->second.precedence; > Now the rest of the program gets shorter and more concise:
float Expression::eval() < auto myop; if (myop == operation.end()) < return std::stoi(symbol); >return myop->second.operation(left->eval(), right->eval()); > Expression Expression::parse(std::string s) < s.erase(std::remove(s.begin(), s.end(), ' '), s.end()); return parseRec(s); >Expression Expression::parseRec(std::string s) < std::string::iterator lowest)>; if (precedence(*lowest) == op_precedence::NON_OP) < return Expression(s, nullptr, nullptr); >else < return Expression(std::string(1, *lowest), new Expression(parseRec(s.substr(0, lowest-s.begin()))), new Expression(parseRec(s.substr(lowest-s.begin() + 1))) ); >> The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.
All compliant compilers will generate the equivalent of return 0 at the end of main , so I generally omit that. Others prefer to write it explicitly for stylistic reasons. Whichever you choose, it's useful to know.
The code, as written, abuses std::shared_ptr because no two objects should ever actually share the same Expression . For that reason, what you really should use instead is std::unique_ptr , but that makes things a bit more complex. In particular, we can no longer freely make copies (or they wouldn't be unique!) so we must use C++ move semantics. It's all perhaps a bit much for a beginner, but here's how one would rewrite the relevant portions of the class declaration.
std::unique_ptr left; std::unique_ptr right; Expression(std::string symbol, std::unique_ptr &&left, std::unique_ptr &&right); Now the constructor looks like this:
Expression::Expression(std::string symbol, std::unique_ptr &&left, std::unique_ptr &&right) : symbol(symbol), left(std::move(left)), right(std::move(right)) <> And instead of using new (which is rather rare in modern C++), we use std::make_unique instead:
return Expression(std::string(1, *lowest), std::make_unique(parseRec(s.substr(0, lowest-s.begin()))), std::make_unique(parseRec(s.substr(lowest-s.begin() + 1))) );