-2
\$\begingroup\$

I have this code:

switch (c) {
    case '-': break;
    case '0': e = expr(j).reduce(C1); e=not(e); a = and(a, e); break;
    case '1': e = expr(j).reduce(C1); a = and(a, e); break;
    default: throw new Exception("Unexpected cube value, " + c);
}

The cases 0 and 1 are only different by not. I do not like to encode the necessity to invert into the arguments of the and function,

switch (c) {
    case '-': break;
    case '0': a = and(true, a, e); break;
    case '1': a = and(false, a, e); break;
    default: throw new Exception("Unexpected cube value, " + c);
}

because this will need a conditional if (invert) e = inv(e); in the function, whereas we already established the need to invert when resolved the switch.

I mean that in case1 you know that you do not need any inv. In case0, you know that you need one. If you now merge the cases, you will need one more if to resolve between case1 (inv is not needed) and case0 (inv is needed) once again.

\$\endgroup\$

closed as off-topic by Jamal Sep 9 '15 at 20:04

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Could you provide some more context about this code? What is a and e? What does and though? What does expr(j).reduce(C1) do? You don't need to provide the code for them, just explain what it is doing and what types you are using. \$\endgroup\$ – Simon Forsberg Dec 14 '13 at 16:40
  • \$\begingroup\$ Are a, e booleans or some more exotic structure? \$\endgroup\$ – rolfl Dec 14 '13 at 17:20
  • 1
    \$\begingroup\$ I agree with @SimonAndréForsberg, and I suspect you may have an XY problem. It looks like you are trying to do arithmetic based on a string representation of some numbers. Maybe BigInteger is appropriate? I encourage you to post the entire function, probably as a separate question referencing this one. \$\endgroup\$ – 200_success Dec 14 '13 at 17:30
  • 1
    \$\begingroup\$ @Val if (c == '-') handleThatcase(); else duplicateOfYourPreviousQuestion(); \$\endgroup\$ – Simon Forsberg Dec 14 '13 at 18:11
  • 1
    \$\begingroup\$ @SimonAndréForsberg You are right. \$\endgroup\$ – Val Dec 14 '13 at 18:42
4
\$\begingroup\$

There are two significantly different ways I can think of to rewrite your code.

The first is to put case '0' and case '1' together by letting one case 'fall through', and then using the ternary operator with in the code to see if we should invert or not.

switch (c) {
    case '-': break;
    case '0': // fall through to execute the same code as below
    case '1': 
         e = expr(j).reduce(C1);
         // Using the ternary operator to determine whether or not e should go through not first.
         a = and(a, c == '0' ? not(e) : e);
         break;
    default: throw new Exception("Unexpected cube value, " + c);
}

The other approach is to skip the switch and use if-else

if (c == '0' || c == '1') {
     e = expr(j).reduce(C1);
     a = and(a, c == '0' ? not(e) : e); // Again, using the ternary operator.
}
else if (c != '-') 
    throw new Exception("Unexpected cube value, " + c);

There might be even better ways to rewrite this code, if I would know more context about it.

I agree that you should not move the logic of inverting e to the and method.

Explanation of the ternary operator:

a = and(a, c == '0' ? not(e) : e);

This is the same thing as:

if (c == '0')
     a = and(a, not(e));
else 
     a = and(a, e);

A more detailed explanation can be found on wikipedia

\$\endgroup\$
  • \$\begingroup\$ Your idea to use extra if-else right in the fallthrough case is better than mine to pull if-else into the and. But, it does not resolve my concern that if-else for invert must be resolved by cases and using extra if-else is bad (cyclomatic complexity, pipeline stalls and discards). I know what is the ternary operator. It is the same as if-else and I do not think that it is better than if-else. Thanks. \$\endgroup\$ – Val Dec 14 '13 at 16:50
  • \$\begingroup\$ @Val Your question was quite unclear so it was hard to tell what level of experience you had, and what you are actually asking. \$\endgroup\$ – Simon Forsberg Dec 14 '13 at 17:13
  • \$\begingroup\$ Not clear? Do you see that not in the common pattern? It is not ok. Merging the cases with additional if is also not ok. \$\endgroup\$ – Val Dec 14 '13 at 17:53
2
\$\begingroup\$

Following on from Simon's answer (which I believe will be more than great for 99.99% of users).....

... actually, I have looked though the code, and I don't understand enough of it to make sense of the right answer:

  • why do you do the map-reduce at all if a = and(a,e) will be 'false' if a starts off 'false`?
  • the map-reduce (two method calls) are far, far more expensive than the switch and conditionals outside the methods... this loop does not need to be optimized any more.
  • The 'boolean-like' variables a, e are not well described at all.

Without knowing more about the code and it's context, I can think of only one thing that may help, and this is an extension of Simon's answer:

if (((c - '0')) >> 1 == 0) { // reduce cyclomatic complexity by 1.
    // '0' is 0x30, (c - '0') will end up with 0 for '0' and 1 for '1' - anything else is wrong
    e = expr(j).reduce(C1);
    a = and(a, c == '0' ? not(e) : e); // Again, using the ternary operator.
} else if (c != '-') 
    throw new Exception("Unexpected cube value, " + c);

Any other suggestions would just be wild guesses.

EDIT

public static void char01() {
    for (char c = Character.MIN_VALUE; c <= Character.MAX_VALUE; c++) {
        if ((c - '0') >> 1 == 0) {
            System.out.println("This matches '" + c + "'");
        }
    }
}

produces:

This matches '0'
This matches '1'
\$\endgroup\$
  • \$\begingroup\$ reduce is just a placeholder for some another function. Sorry for a confusing name. \$\endgroup\$ – Val Dec 14 '13 at 18:26
  • \$\begingroup\$ LSB of c-'0' is 0 or 1 for cases 0 and 1. But (c-'0' >> 1 != 0) passes if c is 2 or larger. How do your single out cases 0 and 1 this way? \$\endgroup\$ – Val Dec 14 '13 at 18:40
  • \$\begingroup\$ @Val - it works... do the math: See my edit: \$\endgroup\$ – rolfl Dec 14 '13 at 18:43
  • \$\begingroup\$ @Val perhaps it should be >> 1 == 0 \$\endgroup\$ – Simon Forsberg Dec 14 '13 at 18:44
  • \$\begingroup\$ Ahh... yes.... copy-paste problem/ \$\endgroup\$ – rolfl Dec 14 '13 at 18:45

Not the answer you're looking for? Browse other questions tagged or ask your own question.