Checking opcode values

Question

What troubles me are the empty /**/ brackets. Is there a cleaner way of handling this logic?

if (opcode == OP_0) { /* continue */}
    else if (opcode == OP_1NEGATE) { /* continue */}
    else if (opcode >= OP_1 && opcode <= OP_16) { /* continue */}
    else { throw new Exception("decodeFromOpN called on non OP_N opcode"); }

Answer 1

What you could use is a function that returns whether an opcode is valid.

private boolean isOpCode(Object opCode){
    return opcode == OP_0 || opcode == OP_1NEGATE || (opcode >= OP_1 && opcode <= OP_16);
}

Then in your code you posted, call the function, and throw an exception if it returns false.

if(!isOpCode(opCode)){
    throw new OpCodeInvalidException("decodeFromOpN called on non OP_N opcode");
}

Replace Object with whatever the opcodes are.

And make a new class that extends from Exception. It's generally bad practice to throw Exception.

Answer 2

The else branch can only be executed if all the other conditions are false:

if (opcode != OP_0
  && opcode != OP_1NEGATE
  && (opcode < OP_1 || opcode > OP_16))
{
    throw ...;
}

Answer 3

I don't like the empty blocks after the if statements. Its very confusing, and clutters up your code.

I also don't like that you are throwing a general Exception. Create one and throw that exception instead.

public class InvalidOpCodeException : Exception
{
    private int OpCode { get; private set; }

    public InvalidOpCodeException(int opCode)
        : base(string.Format("OpCode supplied is invalid {0}.", opCode)
    {
        OpCode = opCode;
    }

}

I would change it to just look for the error case

if (opcode != OP_0
    && opcode != OP_1NEGATE
    && (opcode < OP_1 || opcode > OP_16))
{
     throw new InvalidOpCodeException(opcode);
}

// Rest of the code here

Answer 4

It might be more efficient to use a switch statement, then throw the exception if the opcode does not have a statement.

switch (opcode)
{
    case OP_0: { /* do stuffs */}
    case OP_1NEGATE: { /* do other stuffs */}
    ....
    default: { throw new Exception("a no no"); }
}

Answer 5

I'm going to assume that OP_0...OP_16 are contiguous and that OP_0 is the smallest possible value. In that case, it's trivial to collapse the conditions together:

if (opcode > OP_16 && opcode != OP_1NEGATE)
   throw new Exception("decodeFromOpN called on non OP_N opcode");

If, however, OP_0 may not be contiguous with OP_1 or values less than OP_0 are possible, that could fail.

Answer 6

If you separated the three cases because the opcode values/ranges mean different things, and you chose the same action (accept/skip) for different reasons, I think your style is fine. A comment describing the meaning of each case is useful for the next person to read the code:

if (opcode == OP_0) { /* OP_0 (zero) is a no-op, ok */}
    else if (opcode == OP_1NEGATE) { /* flip register value */}
    else if (opcode >= OP_1 && opcode <= OP_16) { /* known allocated opcodes */}
    else { throw new Exception("decodeFromOpN called on non OP_N opcode"); }

This way it's clearer that the cases had different meanings when you wrote it. As the code evolves, future maintainers can add/break out/consolidate cases as their meaning changes, and/or add debugging information or change actions for the individual cases if necessary.

However, per others' answers, if the cases have roughly the same meaning in the local context of this code, consolidating the conditions makes sense.