3

I wanted to make one of my code more concise so I wrote this:

while(m <= r) 
    nums[m] ? nums[m] == 1 ? m++ : swap(nums[m], nums[r--]) : swap(nums[m++], nums[l++]);

But it's not working because 'swap' is a void function but 'm++' returns int. ('right operand to ? is void, but left operand is of type int' error). So I'd like to know how can I replace m++ so it's of type void.

I know that I can create a new void function (for example void increase(int &x){x++;}) but I want to keep my code as an one liner.

The best working variant I made is 'swap(nums[m], nums[m++])', which does nothing to my array, but it looks awful. What other functions can I use?

  • 2
    A simple cast to `(void)` could fix your issue: [demo on coliru](http://coliru.stacked-crooked.com/a/e105271bf6154f06). However, the whole expression looks a bit scaring... – Scheff's Cat Aug 21 '20 at 10:22
  • 6
    .... arrr ... concise doesn't always mean better. Always prefer readability. This code would fail any decent code review, even if it would compile. – bolov Aug 21 '20 at 10:44
  • @Scheff: I think using `operator ,` after `swap` to also return `int` would be more in the scaring spirit ;) – Jarod42 Aug 21 '20 at 11:13
  • 1
    You're worried about the wrong thing. This code is absolutely unreadable and, worse, unmaintainable. It should be rewritten with `if` statements. – Pete Becker Aug 21 '20 at 13:46
  • That's a complete abuse of the ternary operator. It's not meant to replace the `if` statement, it's meant to choose between two values. – Mark Ransom Aug 21 '20 at 16:09

2 Answers2

2

I wanted to make one of my code more concise

Baking in a number of side effects into a single expression of nested non-parenthesized ternary operators (making use of implicit conversion to bool) does, if anything, make your code more complex, more bug-prone and may hide the fact that the original code should actually have been broken up and re-factored into something simpler.

Why not favour clarity over over-complex brevity? E.g. starting with the straight-forward approach:

while(m <= r) {
    if (nums[m] != 0) {
        if (nums[m] == 1) {
            ++m;
        }
        else {
            swap(nums[m], nums[r--]); 
        }
    }
    else {
        swap(nums[m++], nums[l++]);
    }
}

which can be re-factored into:

while(m <= r) {
    if (nums[m] != 0) {
        if (nums[m] == 1) {
            ++m;
        }
        else {
            swap(nums[m], nums[r]); 
            --r;
        }
    }
    else {
        swap(nums[m], nums[l]);
        ++m;
        ++l;
    }
}

which can be re-factored into:

while(m <= r) {
    const std::size_t swap_from_idx = m;
    std::size_t swap_with_idx = m;  // default: no swapping.

    if (nums[m] == 1) {
        ++m;
        continue;
    }
    else if (nums[m] == 0) {
        swap_with_idx = l;
        ++l;
    }
    else {
        swap_with_idx = r;
        --r;
        ++m;
    }
    swap(nums[swap_from_idx], nums[swap_with_idx]);
}

or e.g.:

while(m <= r) {
    // No swapping.
    if (nums[m] == 1) {
        ++m;
    }
    // Swapping.
    else {
        const std::size_t swap_from_idx = m;
        std::size_t swap_with_idx = l;
        
        if (nums[m] == 0) {
            ++l;
        }
        else {
            swap_with_idx = r;
            --r;
            ++m;
        }
        swap(nums[swap_from_idx], nums[swap_with_idx]);
    }
}

At this point you may ask yourself is the original loop design is overly complex, and/or if you should break out part of the loop body into a separate utility function.


If your if/else if/else logic is reaching a too high cyclomatic complexity, the answer is seldom to try to hide it by means of a highly complex tenary operator expression, but rather by re-factoring and, if applicable, breaking out some parts in separate functions.

dfrib
  • 70,367
  • 12
  • 127
  • 192
0

If you want the expression m++ to have a side-effect, and have a void type, you can simply cast the expression like this:

(void)m++

which will evaluate the m++ first, and then cast it to void.

Here's a demo.

cigien
  • 57,834
  • 11
  • 73
  • 112
  • Woah, it was so simple. C++ is truly amazing. I found that you can also write it as void(m++). I think this one looks better. – Brega Stanislav Aug 21 '20 at 20:23
  • @BregaStanislav Yeah, they're both equivalent. Pick whichever one looks better :) Make sure to read the advice in other answers as well regarding your code. – cigien Aug 21 '20 at 20:27