Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following code:

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

What troubles me are the empty /**/ brackets.

Is there a cleaner way of handling this logic?

share|improve this question
7  
I see that most answers copy the indentation from the question. I would expect the else if chain to align with the initial if. When I see an indented else I tend to start looking for an if at the same level. In general, indent only once within a control structure, but keep the control structure keywords at the level where it starts. –  Henk Langeveld Mar 12 at 11:37

6 Answers 6

up vote 40 down vote accepted

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.

share|improve this answer
25  
The conditional OR operator (||) should be used instead of the bit-wise logical OR operator (|). Semantically it is closer to the meaning, and it can be short-circuited to avoid having to evaluate the entire expression. –  Mark Lakata Mar 11 at 4:59
1  
I see a lot of redundancy here. Why check through each opcode to see if such one exists, only to cycle through again looking for the action it performs? Switching just seems much simpler. –  Liamdev631 Mar 12 at 2:43
    
@Liamdev631, I agree with your idea, to make a switch statement for all of the opcodes, then go from there. However, he asked for a cleaner way of handling the logic. Also, if OP_??? are integers, then calling isOpCode runs relatively quickly. –  WaffleStealer654 Mar 12 at 20:08

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 ...;
}
share|improve this answer

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
share|improve this answer

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"); }
}
share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
2  
This sort of design may be particularly good if the same sequence of tests is done in several methods, and each case has a semantic meaning [possibly explicit no-op]. For example, if the code selects a sequence of if statements based upon an operating mode, a command to "switch to XXX mode (if not already in that mode)" could be handled as a no-op in the XXX mode if-block, but perform an action for the if blocks associated with other modes. –  supercat Mar 11 at 20:40

protected by rolfl Mar 11 at 21:59

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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