CODEWITHSUNDEEP
c++if-statement
Graziano Bolla
Graziano Bolla

Reputation: 7

Doing the same thing inside various 'if' statements, can be improved?

I was coding when i faced this problem and i would like to know if this is correct or it can be improved for performance reasons:

if (color == 0) {
    if (c.val[0] >= 0 && c.val[0] <= 64) {
        //Black
        Paint(cursor.x + x, cursor.y + y);
    }
}
else if (color == 1) {
    if (c.val[0] >= 64 && c.val[0] <= 128) {
        //Grey
        Paint(cursor.x + x, cursor.y + y);
    }
}
else if (color == 2) {
    if (c.val[0] >= 128 && c.val[0] <= 192) {
        //White Gray
        Paint(cursor.x + x, cursor.y + y);
    }
}

As you can see im doing the exact same thing inside all IF statements and it looks kind weird, the program works as i want but i really think im missing something..

Thanks!

Upvotes: 1

Views: 104

Answers (6)

Richard Hodges
Richard Hodges

Reputation: 69882

Lambdas are (as of c++17) guaranteed to be constexpr (i.e. zero cost abstractions) if possible.

They can sometimes allow more succinct and maintainable expressions of logic:

auto range = [](int color)
{
    auto min = color * 64;
    auto max = min + 64;
    return std::make_tuple(min, max);
};

auto in_range = [](auto val, auto tuple)
{
    auto [min, max] = tuple;
    return val >= min && val <= max;
};

if (in_range(c.val[0], range(color)))
    Paint(cursor.x + x, cursor.y + y);

Upvotes: 0

Evg
Evg

Reputation: 26292

Another solution that closely follows the lines of 2785528's answer but uses a lambda to avoid passing additional arguments to PaintIf:

const auto PaintIf = [&](int min, int max)
{
    if (min <= c.val[0] && c.val[0] <= max) 
        Paint(cursor.x + x, cursor.y + y);
};

switch (color)
{
    case 0: PaintIf(  0,  64); break;
    case 1: PaintIf( 64, 128); break;
    case 2: PaintIf(128, 192);
}

Upvotes: 0

2785528
2785528

Reputation: 5566

I like your original post more than the selection.

Here is a another approach with about the same number of lines ... but I find more readable.

void yourSnippet() {
     switch (color) {
     case 0: { PaintIf(  0,  64); } break; // black
     case 1: { PaintIf( 64, 128); } break; // Grey
     case 2: { PaintIf(128, 192); } break; // White Gray
     } // switch
  }

void PaintIf(int min, int max) {
     if ((c.val[0] >= min) && (c.val[0] <= max)) 
        Paint(cursor.x + x, cursor.y + y);
  }

Upvotes: 0

aschepler
aschepler

Reputation: 72311

This isn't too terrible with such a short piece of code repeated, but in the interest of Don't Repeat Yourself, and in case more code is needed later, you could simply separate the testing for whether something will be done from the actual doing it, using a flag.

bool need_Paint = false;
if (color == 0) {
    // Black?
    need_Paint = (c.val[0] >= 0 && c.val[0] <= 64);
}
else if (color == 1) {
    // Grey?
    need_Paint = (c.val[0] >= 64 && c.val[0] <= 128);
}
else if (color == 2) {
    // White Gray?
    need_Paint = (c.val[0] >= 128 && c.val[0] <= 192);
}

if (need_Paint)
    Paint(cursor.x + x, cursor.y + y);

(Note a compiler's optimizations can possibly handle this sort of thing without actually putting a bool in memory, just jumping to the function call statement if the appropriate condition is true.)

(This also looks like the sort of pattern often written as a switch. You could consider using that syntax instead of all the ifs.)

But in fact this particular code can get even simpler, because there's a straightforward mathematical pattern to the three different tests you're doing:

if (color >= 0 && color <= 2 && // (if these aren't already guaranteed)
    c.val[0] >= 64*color && c.val[0] <= 64*(color+1)) {
    Paint(cursor.x + x, cursor.y + y);
}

(Another thing to consider might be using enum ColorType { BLACK=0, GREY=1, WHITE_GRAY=2 }; instead of just writing the "magic numbers" 0, 1, and 2 directly. But if you use both an enum and the mathematical version of this code, I'd recommend you specify the exact values as shown, even though the default for an enum is always consecutive counting from zero, to make it clear you're counting on those values.)

Upvotes: 0

adrtam
adrtam

Reputation: 7211

Because of the structure,

if (color >= 0 && color <= 2) {
    if (c.val[0] >= 64*color && c.val[0] <= 64*(color+1)) {
        Paint(cursor.x + x, cursor.y + y);
    }
}

Upvotes: 5

john
john

Reputation: 87959

A few logical operations make it more compact.

        if ((color == 0 && c.val[0] >= 0 && c.val[0] <= 64) ||
            (color == 1 && c.val[0] >= 64 && c.val[0] <= 128) ||
            (color == 2 && c.val[0] >= 128 && c.val[0] <= 192)) {
                Paint(cursor.x + x, cursor.y + y);
        }

Upvotes: 2

Related Questions