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.
if( (scene==1) || (scene==2) || (scene==3) || (scene==4) || .... (scene==n) ) {
    ....
    ....
}

How can we optimize the condition in the if statement? Any better logic to get the same result?

Any help in this regard will be well appreciated. If this question is below standard, then extremely sorry for that.

share

migrated from stackoverflow.com Jun 12 '12 at 14:01

This question came from our site for professional and enthusiast programmers.

locked by 200_success Jun 26 at 18:55

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.

4  
What is the condition, scene > 0? Needs a lot more info. –  Ryan Jun 12 '12 at 13:58
6  
How to accept Answers –  Levon Jun 12 '12 at 14:00
3  
hey @abhishek-kumar-gupta you should go back and accept a couple of answers to your question. People worked hard on giving you feedback, so you should accept the answers that you felt were most helpful. –  Hans Jun 12 '12 at 14:00
1  
Welcome to Code Review.sx! Your question was migrated here from Stack Overflow. Please register on this site, too, and make sure that both accounts are associated with each other (by using the same OpenID), otherwise you won't be able to comment on or accept answers or edit your question. –  Martin Schröder Jun 13 '12 at 17:09
comments disabled on deleted / locked posts

7 Answers 7

With these values you could try something like this :

if(scene >= 1 && scene <= n) {
    ...
}

If your values are not contiguous, see solutions suggested in Stephen C's or Donald.McLean's post.

share
add comment

For ranges of contiguous ranges of values, Autar's answer is your best bet.

For non-contiguous ranges, or for more complex values (Strings), I would suggest a Set.

private Set<Integer> criteriaValues;

// then in a constructor or initialization block
criteriaValues = new HashSet<Integer>();
// add some values to the set here.

The comparison becomes:

if (criteriaValues.contains(scene)) {
    ...
}
share
1  
This will be slower than a switch. –  Martin Schröder Jun 13 '12 at 17:06
7  
The relative cost will be irrelevant in most programs. Even worrying about such a thing is c2.com/cgi/wiki?PrematureOptimization –  Donald.McLean Jun 13 '12 at 17:12
1  
switch can be optimized by the compiler and the JIT. And programmers know the concept of switch. Of course this only works for objects that switch can handle. –  Martin Schröder Jun 13 '12 at 17:15
7  
If they're writing in Java, they need to learn to use Maps and other commonly used data structures. Also, performance and optimization are almost always irrelevant at this level. Even WORRYING about performance at this level is a Bad Idea. –  Donald.McLean Jun 13 '12 at 17:21
    
But the code needed to FILL the HashSet (in addition to all the necessary but invisible boxing and unboxing) is less readable and longer than that of a switch statement. IMHO, switch clearly wins - better structure, shorter code, no overhead. It's not prematurely optimized. A HashSet is the best fit if you take the values from an external source and cannot hardcode them. –  Arne Aug 24 '12 at 15:24
show 5 more comments

Another alternative is to use a switch statement:

switch (scene) {
case 1:
case 2:
case 3:
case 42:
     // do something
     break;
default:
     // do something else
}

Which approach is best (i.e. most readable) will depend on the set of values you need to test against. I wouldn't worry about efficiency at this level, unless your profiling tells you that this is a significant hotspot.

share
2  
This is great advice. Early on, I worried way too much about saving the processor a little time. –  Evik James Jun 12 '12 at 20:37
add comment

Assuming scene isn't an int,

if ( scene == (int) scene && (scene >= 1 && scene <= n) ){
    ...
}
share
    
ClassCastException? –  Florian Salihovic Jun 12 '12 at 19:50
1  
@FlorianSalihovic Your point? Nothing there will cause a ClassCastException. –  EJP Jun 12 '12 at 22:35
    
I honestly don't understand the casting part's purpose. –  Thomas Eding Jun 13 '12 at 5:22
    
The idea is that scene would be a double or similar. If the double is an int value, ie. 1.0, the statement will return true. –  SimplyPanda Jun 13 '12 at 12:57
1  
That is not very programmer-friendly code. –  Sulthan Jun 17 '12 at 14:24
add comment

I generally do something like:

vals = [ 1 , 2 , 3 , 4 ]
if ( scene in vals) ...
share
    
That looks more like Python than Java. –  Eva Jan 16 '13 at 11:54
add comment

I think, such long condition like this should be the method. In method you can use any way to determine that condition is true. I would prefer the searching in the array.

boolean sceneIsSomething(int scene) {
    int somethings = [ 1, 2, 3, 4 ];
    return somethings.contains(scene);
}

if (sceneIsSomething(scene)) {
    // do something you want ...
}
share
add comment

I strongly recommend Enums for that kind of statements. For example you can create an enum Scene where you map 1,2,3 ... to real instances as ONE, TWO, ... When implementing your if statement you implement a method in your enum. For example:

if(scene.isSceneRelevantForThis()){
  // do stuff
}
share
add comment