Делать одно и то же внутри различных операторов «если», можно улучшить?

Question

Я кодировал, когда столкнулся с этой проблемой, и я хотел бы знать, если это правильно, или это может быть улучшено по соображениям производительности:

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

Как вы можете видеть, я делаю то же самое внутривсе заявления IF, и это выглядит немного странно, программа работает, как я хочу, но я действительно думаю, что я что-то упустил ..

Спасибо!

Answer 1

Другое решение, которое следует за строками ответа 2785528 ​​ , но использует лямбду, чтобы избежать передачи дополнительных аргументов 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);
}

Answer 2

Лямбды (по состоянию на c ++ 17) гарантированно будут constexpr (т. Е. Абстракции с нулевой стоимостью), если это возможно.

Иногда они могут допускать более краткие и поддерживаемые выражения логики:

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);

Answer 3

Мне нравится ваш оригинальный пост больше, чем выбор.

Вот еще один подход с примерно таким же количеством строк ... но я нахожу более читабельным.

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

Answer 4

Это не так уж страшно с таким коротким фрагментом кода, но в интересах «Не повторяйся», и в случае, если позже потребуется больше кода, вы можете просто разделить тестирование на предмет того, будет ли что-то сделаноот фактического выполнения этого с использованием флага.

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);

(Обратите внимание, что оптимизация компилятора может обрабатывать подобные вещи, фактически не помещая bool в память, просто переходя к оператору вызова функции, еслиСоответствующее условие истинно.)

(Это также выглядит как шаблон, часто записываемый как switch. Вы можете использовать этот синтаксис вместо всех if s.)

Но на самом деле этот конкретный код может стать еще проще, потому что есть три простых теста, которые вы делаете: математический шаблон:

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

(Еще одна вещь, которую следует учитывать, - это использовать enum ColorType { BLACK=0, GREY=1, WHITE_GRAY=2 }; вместопросто записать «магические числа» 0, 1 и 2 напрямую, но если вы используете и enum, и математическоеверсию этого кода, я бы порекомендовал вам указать точные значения, как показано, хотя по умолчанию для enum всегда идет последовательный отсчет от нуля, чтобы было ясно, что вы рассчитываете на эти значения.)

Answer 5

Из-за структуры,

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

Answer 6

Несколько логических операций делают его более компактным.

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

Answer 7

vector<int> limits = {0, 64, 128, 192};

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

Способ сделать это заключается в следующем: в коде не должно быть безымянных констант (ну, по крайней мере, лучше их избегать). Затем вы заметите, что их несколько, поэтому логично хранить их в массиве.