Return to Revisions

###Common beginner mistakes

Stop doing this:

using namespace std;

See Why is "using namespace std;" considered bad practice?

###Namespaces All your functions seem to have the prefix rdo_

bool rdo_ws(char c)
char rdo_expr_item_type(char c)
string rdo_opp_to_string(char opp)
bool rdo_is_num(string is_num)
void rdo_count_opp(bool to_count_or_not)
string rdo_eval(string expr)

This is old school C classic from before the time of namespaces. Easier to not use the prefix and put all your functions into a namespace called rdo.

###Maintainability You have a seriously long function rdo_eval() (344 lines). Functions that long are considered bad style as they are really hard to read and maintain. A good rule of thumb is that a function should be no longer than your screen (ie you want to be able to read the whole function in a single glance without scrolling), so 40-80 lines.

###Variables Declare variables where you are going to use them, not at the top of the function. It makes it easy to see usage if you can look close to where you are using the variable to see its definition. This becomes a lot more important when you start using objects with constructors (as the constructors can call arbitrator code).

###Boolean expressions.

Don't use 0 and 1 when you can use true and false (the latter is much more readable).

Don't use an if condition to set a boolean variable (use the boolean condifiton of the if stement as the result).

bool rdo_ws(char c) {
 if (c != ' ')
 return 0;
 else
 return 1;
}
// or
bool rdo_ws(char c)
{
 return c == ' '; // return true if it was a space
}
// or better yet use a built-in test for space.
bool rdo_ws(char c)
{
 return std::is_space(c); // return true if it was a white space
}

###Use a default in your switch If your expression does not match any of the case statments and there is no default then it is undefined behavior. The input may not be legal in your expression but you should detect and report that not go into undefined territory.

char rdo_expr_item_type(char c) {
 switch(c) {
 case '0':
 .....
 case '9':
 case '.':
 c = 'n';
 break;
 case '+':
 ......
 case '/':
 c = 'o';
 break;
 case '(':
 case ')':
 c = 'b';
 break;
 // Whoops no default.
 // potentially very bad.
 }
 return c;
}

OK so we see you are trying for speed here.
So why not just build the result from an array":

char rdo_expr_item_type(char c)
{
 static char const result[] = {
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 // . is 46
 // * (42) + (43) - (45) / (47)
 // ( (40) ) (41)
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'b', 'b', 'o', 'o', 'e', 'o', 'n', 'o',
 // 0 is 48
 'n', 'n', ' n', 'n', 'n', 'n', 'n', 'n', 'n', 'n', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 'e', 'e', ' e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e', 'e',
 };
 return result[static_cast<unisgned char>(c)];
}