Updating images in a weather app

Question

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?

Does objective C allow switch-case with strings like the newer versions of Java? If it does I would use switch-case. — Jon Taylor, Jul 12, 2012 at 14:12
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? — Florent, Jul 12, 2012 at 14:15
@Programmer20005 use [NSString integerValue] for a switch statement. — Christoph Winkler, Jul 12, 2012 at 14:18
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 — jk., Jul 12, 2012 at 15:13
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. — 200_success, Nov 4, 2015 at 3:52

Billal Begueradj · Accepted Answer · 2018-01-05 03:20:05Z

17

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]];

  ...
}

edited Jan 5, 2018 at 3:20

Billal Begueradj

3,4179 gold badges22 silver badges33 bronze badges

answered Jul 12, 2012 at 14:27

mmmmmm

2671 silver badge6 bronze badges

\$\begingroup\$ This seems like the best way so far, I'll try it out and come back! Thanks! \$\endgroup\$
– VijayS.
Jul 12, 2012 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, 2012 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, 2012 at 15:46

Add a comment |

NirmaNirma · Accepted Answer · 2012-07-12 14:44:25Z

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


}

Stefan · Accepted Answer · 2012-07-12 14:44:39Z

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

Bruno Ferreira · Accepted Answer · 2017-06-16 23:10:36Z

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"];
    }
    ...

Christoph Winkler · Accepted Answer · 2012-07-12 14:13:45Z

3

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]];

answered Jul 12, 2012 at 14:13

Christoph Winkler

1311 bronze badge

\$\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\$
– mmmmmm
Jul 12, 2012 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, 2012 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, 2012 at 14:16

Add a comment |

Community · Accepted Answer · 2017-05-23 12:40:52Z

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{
    ...
}
```

Pfitz · Accepted Answer · 2012-07-12 14:14:53Z

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

This is the best answer here. The image name extensions are unnecessary though, just use [UIImage imageNamed:@"Sunny"] etc — trapper, May 25, 2018 at 8:40

Philip Regan · Accepted Answer · 2012-07-12 14:14:06Z

2

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;

answered Jul 12, 2012 at 14:14

Philip Regan

1214 bronze badges

Add a comment |

WidorWidor · Accepted Answer · 2012-07-12 14:14:28Z

2

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.

answered Jul 12, 2012 at 14:14

Widor

Add a comment |

scaryrawr · Accepted Answer · 2012-07-12 14:18:53Z

2

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.

answered Jul 12, 2012 at 14:18

scaryrawr

1212 bronze badges

Add a comment |

Bruno · Accepted Answer · 2012-07-12 14:20:36Z

2

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 (...) {
        }
    }
}

answered Jul 12, 2012 at 14:20

Bruno

1214 bronze badges

Add a comment |

woz · Accepted Answer · 2012-07-12 14:17:08Z

2

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.

answered Jul 12, 2012 at 14:17

woz

1213 bronze badges

\$\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, 2012 at 14:18
\$\begingroup\$ @JonTaylor It's logically the same, but so much more readable. \$\endgroup\$
– woz
Jul 12, 2012 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, 2012 at 14:21
\$\begingroup\$ @JonTaylor Ok, I improved my answer. \$\endgroup\$
– woz
Jul 12, 2012 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, 2012 at 16:46

| Show 1 more comment

dengApro · Accepted Answer · 2018-01-05 01:30:20Z

2

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;
    }
}];

answered Jan 5, 2018 at 1:30

dengApro

3813 silver badges17 bronze badges

Add a comment |

trapper · Accepted Answer · 2018-05-28 08:52:16Z

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

Move this logic out of the VC and into a model object.
Get rid of string comparisons, so convert to a number and use switch()
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.
UIImage imageNamed: doesn't require file type extensions so you can remove them all.
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

user27411user27411 · Accepted Answer · 2013-07-18 08:15:53Z

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.

Stack Exchange Network

Updating images in a weather app

15 Answers 15

Your Answer

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

Hot Network Questions

Updating images in a weather app

15 Answers 15

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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