19
\$\begingroup\$

Right now in my weather app, I have one section of code in my View Controllers to set up the condition image and the background image. It has about 400 lines of if-else statement at the end.

The app's performance is fine, but is it bad to have this? Would it be something that, say, Apple would consider rejecting? The code is very easy to read, and makes perfect sense in my opinion.

- (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, I have a lot of nearly identical statements, which is because I have a lot of conditions. Is it bad practice to do this? If it is, then how can this become more efficient?

\$\endgroup\$

migrated from stackoverflow.com Jul 12 '12 at 17:15

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

  • 5
    \$\begingroup\$ Does objective C allow switch-case with strings like the newer versions of Java? If it does I would use switch-case. \$\endgroup\$ – Jon Taylor Jul 12 '12 at 14:12
  • 4
    \$\begingroup\$ Objective-C doesn't support switch on NSString. But your code would be more readable if you write your conditions like this: Can Objective-C switch on NSString? \$\endgroup\$ – Florent Jul 12 '12 at 14:15
  • 3
    \$\begingroup\$ @Programmer20005 use [NSString integerValue] for a switch statement. \$\endgroup\$ – Christoph Winkler Jul 12 '12 at 14:18
  • 1
    \$\begingroup\$ why do you care about the performance of this branching? it almost certainly isn't a bottleneck compared with actually reading a jpg off disc and displaying it? use the most readable implementation (a dictionary imho) and then profile \$\endgroup\$ – jk. Jul 12 '12 at 15:13
  • 2
    \$\begingroup\$ Also, "what is best practice?" questions are off-topic for Code Review. Reverted to Rev 2 to keep it a "does my code follow best practices?" question. \$\endgroup\$ – 200_success Nov 4 '15 at 3:52

15 Answers 15

17
\$\begingroup\$

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 changes.

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 NSDictionaries (or NSArrays if the conditions are 0 to a number) and then do a straight 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 *)condition {
    conditionsImageView.image = [UIImage imageNamed:
                   [conditionsImages objectForKey:condition]];
    BGView.image = [UIImage imageNamed:[otherImages objectForKey:condition]];

  ...
}
\$\endgroup\$
  • \$\begingroup\$ This seems like the best way so far, I'll try it out and come back! Thanks! \$\endgroup\$ – VijayS. Jul 12 '12 at 14:29
  • \$\begingroup\$ A dictionary is definitely the DRYest method. Perhaps also, since there is a limited amount of pictures, make a second dictionary with image description and image path, so that if the path changes, the english name remains: pictograms = dictionary("sun" => "/path/to/lage-sun.png); conds = dictionary("113" => "sun"); \$\endgroup\$ – jurgemaister Jul 12 '12 at 14:45
  • 1
    \$\begingroup\$ It's not just the indentation. The code really does have nested ifs, as if the programmer was not aware of the "else if" construct. \$\endgroup\$ – Alex Feinman Jul 12 '12 at 15:46
13
\$\begingroup\$

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


}
\$\endgroup\$
8
\$\begingroup\$

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
  }
}
\$\endgroup\$
6
\$\begingroup\$

In my opinion, if a switch is not an option, there is nothing wrong with multiple ifs, I would just rewrite the code 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"];
    }
    ...
\$\endgroup\$
3
\$\begingroup\$

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]];
\$\endgroup\$
  • \$\begingroup\$ Probably because one file e.g. Overcast.jpg is used for more than one number - so only works if you can use hard links in the file system \$\endgroup\$ – user151019 Jul 12 '12 at 14:15
  • 1
    \$\begingroup\$ This one is actually a very good response. Since the images are static, this is the best approach ever possible and saves from redundancy of switch and if-else statements. +1 \$\endgroup\$ – Eugene Jul 12 '12 at 14:15
  • \$\begingroup\$ @Mark of course this could happen. Nevertheless the example states nothing like this. Exceptions could be hardcoded. \$\endgroup\$ – Christoph Winkler Jul 12 '12 at 14:16
3
\$\begingroup\$

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{
        ...
    }
    
\$\endgroup\$
3
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ This is the best answer here. The image name extensions are unnecessary though, just use [UIImage imageNamed:@"Sunny"] etc \$\endgroup\$ – trapper May 25 '18 at 8:40
2
\$\begingroup\$

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;
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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 (...) {
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ else if is what he has already the { } make no difference since its a single line executed after the else. \$\endgroup\$ – Jon Taylor Jul 12 '12 at 14:18
  • \$\begingroup\$ @JonTaylor It's logically the same, but so much more readable. \$\endgroup\$ – woz Jul 12 '12 at 14:19
  • \$\begingroup\$ True, you didn't really explain this in your post though, it seemed more like you were giving it as an alternative when actually its essentially a code style difference. \$\endgroup\$ – Jon Taylor Jul 12 '12 at 14:21
  • \$\begingroup\$ @JonTaylor Ok, I improved my answer. \$\endgroup\$ – woz Jul 12 '12 at 14:33
  • \$\begingroup\$ @ woz: In your comment, you have mentioned that: "A switch statement is really no faster"...Can you please explain? \$\endgroup\$ – Vikram Jul 12 '12 at 16:46
2
\$\begingroup\$

You can put the resources in a dictionary, and loop it.

[@{@"113": @"Sun.png", @"116": @"Mostly_Sunny.png", ...} enumerateKeysAndObjectsUsingBlock:^(id  _Nonnull key, id  _Nonnull obj, BOOL * _Nonnull stop) {
    if ([condition isEqualToString: (NSString *)key]) {
        conditionsImageView.image = [UIImage imageNamed: (NSString *)obj];
        BGView.image = [UIImage imageNamed: (NSString *)obj];
        break;
    }
}];
\$\endgroup\$
1
\$\begingroup\$

You need to do a few things to clean this up.

  1. Move this logic out of the VC and into a model object.
  2. Get rid of string comparisons, so convert to a number and use switch()
  3. All these magic numbers are meaningless without good names so enumerate and name them (WeatherTypeSunny, WeatherTypeMisty, etc) Now they make sense to you, and also to the compiler which can now warn you if any are missing in your switch.
  4. UIImage imageNamed: doesn't require file type extensions so you can remove them all.
  5. You are calling the same image filenames in multiple places so clean that up too (in your helper class)

All that should be left in the VC is something like.

MyWeather *weather = [[MyWeather alloc] initWithCondition:condition.intValue];
conditionsImageView.image = weather.image;
BGView.image = weather.backgroundImage;

Now you can flesh it out with any extra weather related logic too

\$\endgroup\$
0
\$\begingroup\$

I'd suggest a more object oriented solution:


First create a model class ICBWeatherConditionImage:

@property (nonatomic, assign, readonly) NSUInteger identifier;
@property (nonatomic, strong, readonly) UIImage *backgroundImage;
@property (nonatomic, strong, readonly) UIImage *image;

- (id)initWithIdentifier:(NSUInteger)identifier imageName:(NSString *)imageName backgroundImageName:(NSString *)backgroundImageName;

Then create a manager class ICBWeatherConditionImageManager that loads the basic data from an XML file:

- (id)initWithDataAtURL:(NSURL *)url;

It can then search and return the correct ICBWeatherConditionImage object based on a given weatherCondition.

- (ICBWeatherConditionImage *)conditionImageForWeatherCondition:(ICB_WeatherConditions *)weatherCondition;

Finally you have to check whether you create all ICBWeatherConditionImage during the initialization of the manager object or, to reduce memory consumption, create them on the fly.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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