Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
added 330 characters in body
Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338

Also by splitting your function into logic units (smaller functions). If you give each one a meaningful name (and you should) it helps in documenting the code. At the call point you don't have to go and read the code (as the name of the function tells you what is logically happening). Thus it makes the code self documenting.

Also by splitting your function into logic units (smaller functions). If you give each one a meaningful name (and you should) it helps in documenting the code. At the call point you don't have to go and read the code (as the name of the function tells you what is logically happening). Thus it makes the code self documenting.

added 3318 characters in body
Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338

###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)];
}

###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)];
}
Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338
Loading