Conversation
|
|
||
| class IOperandsMemory{ | ||
| public: | ||
| virtual value_t getOperand() = 0; |
There was a problem hiding this comment.
Looks like method should not change class state. May be const?
| public: | ||
| virtual value_t getOperand() = 0; | ||
| virtual void putOperand(const value_t&) = 0; | ||
| virtual void applyOperation(operation_t) = 0; |
There was a problem hiding this comment.
Not clear output of the method
| @@ -0,0 +1,24 @@ | |||
| #include "operandsmemory.h" | |||
|
|
|||
| PolishCalcComponent::value_t PolishCalcComponent::OperandsMemory::getOperand() | |||
There was a problem hiding this comment.
I would rename to Pop operand
|
|
||
| class OperandsMemory: public IOperandsMemory{ | ||
| public: | ||
| virtual value_t getOperand() override; |
There was a problem hiding this comment.
C++ note.
Virtual and override keywords on the same place are redundant.
| using value_t = double; | ||
|
|
||
| class IOperandsMemory; | ||
| using operation_t = std::function<void(PolishCalcComponent::IOperandsMemory*)>; |
There was a problem hiding this comment.
operation_t naming (OperationType) is a bit confusing.
I would rename it just to an Operation
There was a problem hiding this comment.
as default, for typedef and using i use *_t
| std::vector<std::string> tokens; | ||
| PolishCalcComponent::split2Tokens(inputStr, tokens); | ||
|
|
||
| std::unique_ptr<IOperandsMemory> memory(makeMemory()); |
There was a problem hiding this comment.
C++ suggest :
auto memory = make_memory()
|
|
||
| std::unique_ptr<IOperandsMemory> memory(makeMemory()); | ||
|
|
||
| processTokens(tokens, memory.get()); |
There was a problem hiding this comment.
if processTokens does not control memory life-cycle I would provide it by reference
|
|
||
| processTokens(tokens, memory.get()); | ||
|
|
||
| assert(1 == memory->size()); |
There was a problem hiding this comment.
It depends on user input. Please do not assert user input
|
|
||
| // Decompose statement | ||
| while( pos != std::string::npos ) { | ||
| out.push_back( in.substr( initialPos, pos - initialPos ) ); |
There was a problem hiding this comment.
I would make microrefactoring of this method and use std lib
|
|
||
| //TODO: fix negative tests | ||
|
|
||
| //TEST_F(CalcTest, LogicErrorExtraOperator) { |
3 participants
No description provided.