-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Calc #9
base: master
Are you sure you want to change the base?
Calc #9
Conversation
f7c8c12
to
da3665e
Compare
{ | ||
EXPECT_EQ(7.8, project_.calc("5.5 2.3 +")); | ||
} | ||
TEST_F(CalculatorTest, MissingOperand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negative and positive test cases.
test/calculator_test.cpp
Outdated
} | ||
TEST_F(CalculatorTest, MissingOperand) | ||
{ | ||
EXPECT_EQ(-1, project_.calc("5 +")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check a way to handle errors
@@ -46,53 +46,21 @@ namespace dev | |||
double PolishCalc::calc(const std::string &str) | |||
{ | |||
if (str.size() == 0) | |||
return 0; | |||
throw Exception{"Nothing to work on as Input is empty"}; | |||
for (int i = 0; i < str.size(); i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to use iterator or range based loop?
polishNotationCalc.cpp
Outdated
// { | ||
// std::cout << "calc::Exception" << e.what(); | ||
// } | ||
res = performOperation(str[i], operandVal.first, operandVal.second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next step is to separate parsing and evaluating
polishNotationCalc.cpp
Outdated
@@ -101,4 +69,36 @@ namespace dev | |||
|
|||
return mStack.top(); | |||
} | |||
|
|||
void PolishCalc::pushOperandInStack(const std::string &str, int &curr_pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- mark variable as output explicit with name
curr_pos_out
- return new position
polishNotationCalc.cpp
Outdated
{ | ||
double secOperand = 0; | ||
double firstOperand = 0; | ||
if (mStack.size() >= 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swap if condition and make early return/ throw ( safety check)
polishNotationCalc.hpp
Outdated
std::stack<double> mStack; | ||
std::map<const char, functionCall> mMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mMap bad name. Try to avoid put type name in variable, Show business value of variable
double performOperation(const char operation, const double firstOperand, const double secOperand); | ||
void pushOperandInStack(const std::string &, int& curr_pos); | ||
std::pair<double,double> getOperandFromStack(); | ||
void insertFunctionInsideMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to use constexpr
for this.
polishNotationCalc.cpp
Outdated
@@ -59,7 +36,7 @@ namespace dev | |||
{ | |||
auto operandVal = getOperandFromStack(); | |||
double res = 0; | |||
res = performOperation(str[i], operandVal.first, operandVal.second); | |||
res = mMap[str[i]](operandVal.first, operandVal.second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract variable for str[i]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to split implementation to parsing, validating and calculating steps
mMap.insert(std::make_pair('-', [](double a, double b) { return a - b; })); | ||
mMap.insert(std::make_pair('*', [](double a, double b) { return a * b; })); | ||
mMap.insert(std::make_pair('/', [](double a, double b) { | ||
if(b==0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(0=b)
polishNotationCalc.cpp
Outdated
{ | ||
mMap.insert(std::make_pair('+', [](double a, double b) { return a + b; })); | ||
mMap.insert(std::make_pair('-', [](double a, double b) { return a - b; })); | ||
mMap.insert(std::make_pair('*', [](double a, double b) { return a * b; })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use const as wide as you cat
throw Exception{"Nothing to work on as Input is empty"}; | ||
for (int i = 0; i < str.size(); i++) | ||
{ | ||
if (str[i] == ' ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid missing brackets even in one line if/else/while ...
return false; | ||
} | ||
|
||
double PolishCalc::calc( const std::string &str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 blocks :
1. Basic Validation // Check that the is not bad symbols and so on
2. Parsing // Use not strings, but OOP types or lambdas for futher working
2.1 Validation : count of operator matches count of operands
3. Calculation // Evaluete expression
2 stacks :
Validation block fill up 2 stacks :
- Operator stack
- Operand stack
Calculation should work with Operator/Operand or lambda types but not string.
{ | ||
if (str[i] == ' ') | ||
continue; | ||
if (isNumeric(str[i])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to modify solution to work with negative numbers
{ | ||
double operand = 0; | ||
std::string temp = ""; | ||
while (curr_pos < str.size() && !(str[curr_pos] == ' ')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of input like 2 4+
, solution behaviour is undefined.
Try to handle this error.
Best option is to create prevalidation function and contract for pushOperandInStack.
continue; | ||
if (isNumeric(str[i])) | ||
{ | ||
pushOperandInStack(str, i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to work with some kind of tokens but not charters.
It is not obvious that pushOperandInStack will modify i
variable. If it is possible. Try to avoid it
pushOperandInStack(str, i); | ||
|
||
} | ||
if (isOperator(str[i])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create separate loop to fill operator stack, operand stack an calculation based on 2 stacks
{ | ||
auto operandVal = getOperandFromStack(); | ||
double res = 0; | ||
res = mOperatorMap[str[i]](operandVal.first, operandVal.second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move calculation to the third loop
if (num == '+' || num == '-' || num == '*' || num == '/') | ||
return true; | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (from 0 to N) {
Operation 1
Operation 2
Operation 3
}
for (from 0 to N) {
Opertation 1
}
for (from 0 to N) {
Opertation 2
}
for (from 0 to N) {
Opertation 3
}
Both solutions have the same performance
{ | ||
insertFunctionInsideMap(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 Step : Create Token stack and validate it
token_stack& = stack<Tocken>
Token {
// Process one tocken in the stack
void process(token_stack& )
double value(const token_stack& ) << // Follow contant
}
NumericToken : Token {
void process(token_stack& ) -> do nothing
double value(token_stack& ) -> return value_
double value_
}
OperatorTocken : Token {
pair PopTockens
}
PlusTocken : OperatorTocken {
void process(token_stack& ) {
pop token from stack -> get value -> double res1
pop token2 from stack -> get value -> double res2
res1 + res2 -> Plus result
Push plus result to token vector as Numeric
}
double value(const token_stack&) {
// Option 1 Contract thet process() should be evaluated before value ()
return top of the token_stack& value()
// Option 2 Base that there process() could not be called
get token from stack -> get value -> double res1
get token2 from stack on the second position -> get value -> double res2
res1 + res2 -> Plus result
return Plus result as double
}
}
3 step Go throw all tokens and call process to them
No description provided.