• 1

• 26.2k
• 13
• 47
• 113

##Answers to Specific Questions

There were some functions that weren't part of the "calculator". So I didn't define them in the header file. Is this bad practice or good practice?

It is sometimes necessary to keep some things private, these functions don't need a prototype in the header file.

I'm fairly new to exception handling; Did do it correctly in my code?

The problem with your exception handling is that you are returning the error,what() value as a successful word in at least one case. How does the outer program know that this is an error and stops the processing. It is possible that they try{} catch{} implementation is at a too low level and may need to be at a higher level in the programming so that the program resets and doesn't try to process the string.

##General Observations One thing to always keep in mind when designing and writing software is maintainability. Functionality always grows as a program matures and this means that there are changes that must be made. You may win the lottery or inherit a lot of money so you may not be the one maintaining the code. This code will not be easy to modify by other people.

It also seems that you may have missed some of the suggestions that EmilyL made (make sure your code is working properly).

##Separate Functionality There are 2 sets of functionality here, parsing the expressions and performing the calculations, split the functionality so that first the input is entirely parsed, and then calculate the value if the expression is legal, don't try to do everything at once. This will simplify writing and debugging the code. Build a parse tree of operators and operands and then process the parse tree separately. There is really no need to convert to Reverse Polish Notation internally.

Quite possibly there should be 2 classes used by the program, a parser and then a calculator. The parsing algorithm and the calculating algorithm should be in separate source files with separate header files.

##Complexity The function std::string toRPN(const std::string &expression) is far too complex (does too much in one function) and should have blocks of code broken out into more functions. In the dinosaur age of computing, functions that were more than one page long (about 60 lines) were considered too long to be understandable. In the modern age, any function that does not fit on a screen is too long.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

#Let the Compiler do the Optimization In the C++ language the keyword inline is obsolete. Optimizing compilers do a much better job of deciding what functions should be inlined. There isn't any other use for the keyword inline other than optimization.

#Prefer Data Structures Over Long If Then Else If Statements In the function void applyFunction(std::string &function, NUMTYPE &argument) the very long if then else if statement might be better implemented using std::map. This would make adding or deleting operations much easier because the map is easier to add to. It would also greatly reduce the amount of code in this function.

##Use the Conditional (or Ternary) Operator The functions inline bool isNumber(const char character) inline bool isLetter(const char character) and inline bool isOperator(const char character) could all be much shorter, in the case of isNumber() and isLeter() they are one liners if you use the ternary operator.

inline bool isNumber(const char character)
{
 return ((character >= '0' && character <= '9') || character == '.');
}
inline bool isLetter(const char character)
{
 return ((character >= 'a' && character <= 'z') || (character >= 'A' && character <= 'Z'));
}

If you include <cctype> this becomes even simpler, isLetter() can simply be replaced by isalpha() and isNumber() can be simplified with

inline bool isNumber(const char character)
{
 return (isdigit(character) || character == '.');
}

The function inline bool isOperator(const char character) is easier to maintain if it written in the following manner.

bool isOperator(const char character)
{
 std::vector<char>operators = {'+', '-', '*', '/', '&', '^', '(', ')'};
 for (char m_operator: operators)
 {
 if (m_operator == character)
 {
 return true;
 }
 return false;
 }
}