Bad Practice to have Long If-Else statements?

Question

EDIT: One of the bigger questions I have is that, is using switch-case more efficient then using this?

Right now in my application (a weather app), I have one section of code in my View Controllers to set up the condition image, and the background image. It's about 400 lines of if-else statement at the end. The app's performance is fine and works fast, but is it bad to have this? Would it be something that say Apple would be looking at and would reject? The code is very easy to read, and makes perfect sense in my opinion. Here's a snippet:

NOTE THIS IS BAD CODE, I DID NOT USE ELSE-IF's AND HAD NESTED STATEMENTS, AND IT IS NOT EASILY READABLE, AND HAS DUPLICATED CODE

- (void)updateImages:(ICB_WeatherConditions *)weather {
    if ([condition isEqualToString:@"113"]) {
        conditionsImageView.image = [UIImage imageNamed:@"Sun.png"];
        BGView.image = [UIImage imageNamed:@"Sun.jpg"];
    } else {
        if ([condition isEqualToString:@"116"]) {
            conditionsImageView.image = [UIImage imageNamed:@"Mostly_Sunny.png"];
            BGView.image = [UIImage imageNamed:@"Partly_Cloudy.jpg"];

        } else {
            if ([condition isEqualToString:@"119"]) {
                conditionsImageView.image = [UIImage imageNamed:@"Overcast.png"];
                BGView.image = [UIImage imageNamed:@"Overcast.jpg"];
            }
            else {
                if ([condition isEqualToString:@"122"]) {
                    conditionsImageView.image = [UIImage imageNamed:@"Overcast.png"];
                    BGView.image = [UIImage imageNamed:@"Overcast.jpg"];
                }
                else {
                    if ([condition isEqualToString:@"143"]) {
                        conditionsImageView.image = [UIImage imageNamed:@"Mist.png"];
                        BGView.image = [UIImage imageNamed:@"Foggy.jpg"];
                    }
                    else {
                        if ([condition isEqualToString:@"176"]) {
                            conditionsImageView.image = [UIImage imageNamed:@"Scattered_Thunderstorms.png"];
                            BGView.image = [UIImage imageNamed:@"Scat_Tstorms.jpg"];
                    }

It keeps going on after that. As you can see, the statements are almost identical, and the only reason I have a lot is because I have a lot of conditions. Is it bad practice to do this? and if it is, is there any way that is more efficient then this? Thanks!

Answer 1

- (void)updateImages:(ICB_WeatherConditions *)weather {

    static NSDictionary *map = nil;
    if (map == nil) {
        map = [[NSDictionary dictionaryWithObjectsAndKeys:
            @"Sun,Sun", @"113",
            @"Mostly_Sunny,Partly_Cloudy", @"116",
            @"Overcast,Overcast", @"119",
            ...
            nil] retain];
    }

    NSString *pair = [map objectForKey:condition];
    NSArray *images = [pair componentsSeparatedByString:@","];

    if ([images count] != 2) {

        // handle failed configuration

    } else {

        conditionsImageView.image = [UIImage imageNamed:[images objectAtIndex:0]];
        BGView.image = [UIImage imageNamed:[images objectAtIndex:1]];

    }
}

Answer 2

In this case your indentation makes the code drift over to the right so does make the code difficult to read so does need some change .

First I would note that a lot of code is repeated and also as noted a switch after converting the condition to an int is possible.

However in this case your code is effectively doing multiple lookups so I would look at using NSDictionaies (or NSArrays if the conditions are 0 to a number) and then do a straignt lookup

e.g. setup

NSDictionary* conditionsImages = [NSDictionary dictionaryWithObjectsAndKeys: 
                @"113", @"Sun.png",
                @"116", @"Mostly_Sunny.png",
     ...
                nil];

Or read the dictionaries from a file

Access them by

- (void)updateImages:(ICB_WeatherConditions *)weather {
    conditionsImageView.image = [UIImage imageNamed:
                   [conditionsImages objectForKey:condition]];
    BGView.image = [UIImage imageNamed:[otherImages objectForKey:condition]];

  ...
}

Answer 3

There is a lot of overhead in calling the string methods on the objects + strings can not be used as the condition in switch statements. You are lucky that the values you are driving your logic by are ints. So just convert the value to an int value and run your code in a switch statement like so:

 - (void)updateImages:(ICB_WeatherConditions *)weather {

    //convert string to int value
    NSString *conditionS = [weather condition];
    int var = [conditionS intValue];

    switch(var){
        case 113:
            conditionsImageView.image = [UIImage imageNamed:@"Sun.png"];
            BGView.image = [UIImage imageNamed:@"Sun.jpg"];
        break;
        case 116:
            conditionsImageView.image = [UIImage imageNamed:@"Mostly_Sunny.png"];
            BGView.image = [UIImage imageNamed:@"Partly_Cloudy.jpg"];
            break;
        case 119:
            conditionsImageView.image = [UIImage imageNamed:@"Overcast.png"];
            BGView.image = [UIImage imageNamed:@"Overcast.jpg"];            
            break;

            (etc)
            .
            .
            .
        default:
            (default code)
    }


}

Answer 4

In my opinion (if a switch is not an option), there is nothing wrong with multiple ifs, I just would rewrite in the following manner for better readability and less indentations.

- (void)updateImages:(ICB_WeatherConditions *)weather
{
    if ([condition isEqualToString:@"113"])
    {
        conditionsImageView.image = [UIImage imageNamed:@"Sun.png"];
        BGView.image = [UIImage imageNamed:@"Sun.jpg"];
    }
    else if ([condition isEqualToString:@"116"])
    {
        conditionsImageView.image = [UIImage imageNamed:@"Mostly_Sunny.png"];
        BGView.image = [UIImage imageNamed:@"Partly_Cloudy.jpg"];
    }
    else if ([condition isEqualToString:@"119"])
    {
        conditionsImageView.image = [UIImage imageNamed:@"Overcast.png"];
        BGView.image = [UIImage imageNamed:@"Overcast.jpg"];
    }
    else if ([condition isEqualToString:@"122"])
    {
        conditionsImageView.image = [UIImage imageNamed:@"Overcast.png"];
        BGView.image = [UIImage imageNamed:@"Overcast.jpg"];
    }
    else if ([condition isEqualToString:@"143"])
    {
        conditionsImageView.image = [UIImage imageNamed:@"Mist.png"];
        BGView.image = [UIImage imageNamed:@"Foggy.jpg"];
    }
    else if ([condition isEqualToString:@"176"])
    {
        conditionsImageView.image = [UIImage imageNamed:@"Scattered_Thunderstorms.png"];
        BGView.image = [UIImage imageNamed:@"Scat_Tstorms.jpg"];
    }
    ...

Answer 5

Put your image mapping in a property list:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
  <key>113</key>
  <dict>
    <key>condition</key>
    <string>Sun.png</string>
    <key>background</key>
    <string>Sun.jpg</string>
  </dict>
  <key>116</key>
  <dict>
    <key>condition</key>
    <string>Mostly_Sunny.png</string>
    <key>background</key>
    <string>Partly_Cloudy.jpg</string>
  </dict>
</dict>
</plist>

And rewrite your method:

- (void)updateImages:(ICB_WeatherConditions *)weather {
  NSDictionary *dict = [NSDictionary dictionaryWithContentsOfFile:@"your.plist"];
  NSDictionary *images = [dict objectForKey:condition];

  if (images) {
    conditionsImageView.image = [UIImage imageNamed:[images objectForKey:@"condition"]];
    BGView.image = [UIImage imageNamed:[images objectForKey:@"background"]];    
  }
  else {
    // fallback
  }
}

Answer 6

Why don't you just rename your Files to the matching Number?

conditionsImageView.image = [UIImage imageNamed:[NSString stringWithFormat:@"%@.png", condition]]; 
BGView.image = [UIImage imageNamed:[NSString stringWithFormat:@"%@.jpg", condition]];

Answer 7

I'd say a switch statment would be preferable here, especially as you are performing the If on the same expression each time.

Were all the If conditions different variables then I'd say keep it as it is, but a switch is ideal here.

Answer 8

You should combine your else and if so you don't over nest since it's all at the same level.

    if (condition) {
        // Do something.
    } else if (someOtherCondition) {
        // Something else.
    }

Another thing you could do (it would just have a long piece of code else where) is to set up a Dictionary, I think you could even use an XML file or something to load and it would be a bit cleaner and shorter.

Answer 9

If you can convert the strings to integers, it would be more readable if you use the switch statement.

Otherwise, here's a similar question, with several different solutions offered. I'd suggest shipping as is, since you've already measured performance and it's fine.

Alternative solutions include

• using enums (which you can switch over)
• having the strings be values in a dictionary (with ints as keys that you can switch over).

• using elseif to make the indentation look nicer:

  if { 
      ...
  else if{
      ...
  }
  

Answer 10

The main readability problem here has to do with the indentation.

Don't open a bracket after the else and put the if statement directly:

if (...) {
} else if (...) {
} else if (...) {
}

instead of:

if (...) {
} else {
    if (...) {
    } else {
        if (...) {
        }
    }
}

Answer 11

In theory there is nothing against such a thing but you are repaeting a lot of code for setting the image therefore i would recomment to use a switch statement instead of the if-elses and make a own method for the inner part of the if-elses. And to make it even more readable use a enum:

The enum:

enum {
    WeatherTypeSun = 115,
    WeatherTypeMostlySunny = 116;
}   WeatherType;

and here the switch statement with use of the enums:

- (void)setWeatherImages:(NSString *)weather {
    conditionsImageView.image = [UIImage imageNamed:[weather stringByAppendingString:@".png"]];
    BGView.image = [UIImage imageNamed:[weather stringByAppendingString:@".jpg"]];
}

- (void)updateImages:(ICB_WeatherConditions *)weather {

    int weatherCondition = [condition intValue];

    switch (weatherCondition) {
        case WeatherTypeSun: 
            [self setWeatherImages:@"sun"];
            break;
        case WeatherTypeMostlySunny:
            [self setWeatherImages:@"mostly_sunny"];
            break;
    }
}

Answer 12

Use else if!

if (foo) {
    //do something
}
else if (bar) {
    //do something else
}

Apple will definitely not reject you for what you have, but using else if will make it much easier to read and get rid of all that indenting. A switch statement is really no faster, and plus you usually have to have a break; for every case, which is kind of silly to me.

Answer 13

You should look at switch statements for this. The template code in Xcode looks something like this:

switch (condition) {
    case 113:
        // do something
        break;
    case 116:
        // do something
        break;
    /* etc. */
    default:
        // when all else fails, do something
        break;