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 created a weather widget in JavaScript that requests the current weather from Yahoo. The weather request returns a code which corresponds with a current weather image. I don't like Yahoo's images, so I use my own CSS classes that correspond with my own images. Consequently, the following code returns the correct CSS class which I then deal with later:

function returnClass(c) {
    if (c === "0" || c === "19" || c === "23" || c === "24") {
        return ["basecloud", "wind"];
    } else if (c === "1" || c === "2") {
        return ["basecloud", "rain"];
    } else if (c === "3" || c === "4" || c === "37" || c === "38" || c === "39" || c === "45" || c === "47") {
        return ["basecloud", "thunder"];
    } else if (c === "5" || c === "6" || c === "7" || c === "8" || c === "10" || c === "18") {
        return ["basecloud", "sleet"];
    } else if (c === "9") {
        return ["basecloud", "drizzle"];
    } else if (c === "11" || c === "12" || c === "40") {
        return ["basecloud", "rain"];
    } else if (c === "13" || c === "14" || c === "15" || c === "16" || c === "41" || c === "42" || c === "43" || c === "46") {
        return ["basecloud", "snow"];
    } else if (c === "17" || c === "35") {
        return ["basecloud", "hail"];
    } else if (c === "20" || c === "21" || c === "22") {
        return ["mist"];
    } else if (c === "25") {
        return ["basecloud", "frost"];
    } else if (c === "26" || c === "27" || c === "29" || c === "33" || c === "28" || c === "30" || c === "34" || c === "44") {
        return ["cloud"];
    } else if (c === "31") {
        return ["moon"];
    } else if (c === "32" || c === "36") {
        return ["sun"];
    } else if (c === "3200") {
        return ["none"];
    }
}

Is there any way to simplify the above code? As you can tell, multiple codes can represent the same CSS class, which is why I have so many or operators. The only requirement is that it returns the correct array depending on the code.

share|improve this question

5 Answers 5

up vote 7 down vote accepted

If you don't mind using the 'JSON strings as index' trick, you can do the following:

var classMap = 
{
  '["basecloud", "wind"]' : [0,19,23,24],
  '["basecloud", "rain"]' : [1,2],
  '["basecloud", "thunder"]' : [3,4,37,38,39,45,47],
  etc. etc  
} 


function returnClass(c)
{
  var n = +c, key;
  for( key in classMap )
    if( ~classMap[key].indexOf( n ) )
      return JSON.parse( key );
  return ["none"];
}

This implies of course that you run on a modern browser, or that you shim in the indexOf ( you can use the code from here ).

A more old skool version of the returnClass function would be

function returnClass(c)
{
  var n = parseInt(c,10), key;
  for( key in classMap )
    if( classMap[key].indexOf( n ) != -1 )
      return JSON.parse( key );
  return ["none"];
}
share|improve this answer
    
+1 - I like this solution, too –  lwburk Dec 23 '13 at 22:33
    
This one seems the cleanest. Thankfully I have the luxury of developing for a specific browser on a specific platform. I also like @lwburk's example as well, and his has the advantage of being more readable. For instance, are you setting n to +c so that it increments each time? And what does the tilde (~) operator accomplish? –  Charlie Dec 24 '13 at 14:30
3  
n = +c converts a string to a number ( '20' to 20 ). ~ ( Bitwise NOT ) effectively converts -1 to 0 and all other values to not-zero. It is an answer as to why indexOf returns -1 when not found. So ~'abc'.indexOf('a') is true, ~'abc'.indexOf('d') is false. –  konijn Dec 24 '13 at 14:37
    
Ah. Thanks for updating the answer with your recent explanation. I like this method the most. –  Charlie Dec 24 '13 at 17:03

This sort of thing, with a finite set of coditions, is typically done using an array:

var codeIcons=[
      ["basecloud", "wind"],
      ["basecloud", "rain"],
      ["basecloud", "rain"],
      ["basecloud", "thunder"],
      ["basecloud", "thunder"],
      ....
    ];

Then your method simply becomes:

function returnClass(c) {
    c = parseInt(c);
    return (c < codeIcons.length) ? codeIcons[c] : ["none"];
}

Just adding to a set of advantages this method has:

  • sure, it is hard-coding logic, but, the presentation is better than multiple if-statements.
  • it is more managable - you can easily add conditions
  • you know exactly what conditions lead to certain output (you do not need to scan pages of code to find the right conditions
  • you cannot have bugs where multiple conditions lead to the same result (these types of bugs tend to creep in after maintaining the code - you have multiple or-conditions where c == xxx.
  • if, further down the road, you want to migrate the logic for what classes/icons to use, and when, you can easily just change where the array-of-values is configured (i.e. it can become a configuration value, not hard-coded.)
share|improve this answer
    
So I will have an array with 36 subarrays? –  Charlie Dec 23 '13 at 18:11
    
@Charlie exactly, and handle the 3200 independently. This will reduce your code substantially. You can choose whether to handle 3200 and everything else as ["none"] or whether to have some error condition for non-3200 values. –  rolfl Dec 23 '13 at 18:22
3  
I think the re-formatted code is not very readable. As array literals don't have index numbers, it's difficult to see which classes correspond to a given number. –  epidemian Dec 24 '13 at 2:09

I like @rolfl's answer. Here's another way to do it that's superficially different, but sort of (but not really) similar. First, create the following function:

var addIcons = function(icons, c) {
    return function(push, values) {
        if (values.indexOf(c) !== -1) {
            icons.push(push);
        }
        return icons;
    }
}

This function constructs a new function that is specifically built to check its inputs for the existence of c and to push a new value into the icons array when it's found. This function closes over both c and icons so the results are accumulated, as expected. Here's an example:

var add = addIcons([], 2);
add("basecloud", [0, 19, 23, 24, 1, 2, 3, 4, 37, 38, 39, 
                  45, 47, 5, 6, 7, 8, 10, 18, 9, 11, 12, 
                  40, 13, 14, 15, 16, 41, 42, 43, 46, 17, 
                  35, 25]);
add("rain", [1, 2]);
add("thunder", [3, 4, 37, 38, 39, 45, 47]);

// ...and so on

Output:

["basecloud", "rain"]

Of course, you can (and probably should) create a lookup table (sort of the inverse of @rolfl's table) and further abstract the code needed to get the final result:

var icons = {
    "basecloud": [0, 19, 23, 24, 1, 2, 3, 4, 37, 38, 39, 
                  45, 47, 5, 6, 7, 8, 10, 18, 9, 11, 12, 
                  40, 13, 14, 15, 16, 41, 42, 43, 46, 17, 
                  35, 25],

    "rain": [1, 2],
    "thunder": [3, 4, 37, 38, 39, 45, 47]
};

function getAllIcons(c) {
    var res = [];
    var add = addIcons([], c);
    for (var icon in icons) {
        res = add(icon, icons[icon]);
    }
    return res;
}

Usage:

getAllIcons(2);

Output:

["basecloud", "rain"]
share|improve this answer
    
This is how I usually do it, sparse arrays like that scale better for more spread out values and translate seamlessly to arbitrary objects instead of just integers. –  Blindy Dec 23 '13 at 20:52
    
I like the second implementation. That way I would only need 15 subarrays instead of 36. Is the return speed negligible between the previously accepted answer and this one? –  Charlie Dec 24 '13 at 14:34
    
This is also way more manageable for future maintenance than rolfl's answer, which doesn't list the indexes. –  Izkata Dec 24 '13 at 16:13
    
@charlie - Array lookup by index is done in constant time, but this needs to loop over each icon type and search its array to see if the value is present, so mine is definitely slower (n * m operations versus a single array lookup). But I think we're talking about a micro-optimization for structures of the size presented. I would go for readability and maintainability first. –  lwburk Dec 24 '13 at 22:18

Why not use the switch-case construct for this?

You could condense your if-else structure into the following:

switch(c)
{
    case '0':
    case '19':
    case '23':
    case '24':
        return ["basecloud", "wind"];

    case '1':
    case '2':
        return ["basecloud", "rain"];

    ...
}
share|improve this answer
    
That's not really saving any lines of code though. –  Charlie Dec 23 '13 at 19:58
    
Yeah, it's fine, but it's going to be looooong. –  lwburk Dec 23 '13 at 20:45
3  
It may be longer, but it will be a lot clearer and that is what matters. –  SQB Dec 24 '13 at 8:00
2  
Not longer than the accepted answer, which is also harder to maintain than the original if-else setup should he wish to change any of the css classes, or the original mapping from the Yahoo feed changes. –  Hayko Koryun Dec 24 '13 at 8:01
    
@HaykoKoryun Good point. –  Charlie Dec 24 '13 at 14:27
// Programmer-friendly representation
var WEATHER = [
    { class: ["basecloud", "wind"],     codes: [0, 19, 23, 24] },
    { class: ["basecloud", "rain"],     codes: [1, 2] },
    { class: ["basecloud", "thunder"],  codes: [3, 4, 37, 38, 39, 45, 47] },
    { class: ["basecloud", "sleet"],    codes: [5, 6, 7, 8, 10, 18] },
    { class: ["basecloud", "drizzle"],  codes: [9] },
    { class: ["basecloud", "rain"],     codes: [11, 12, 40] },
    { class: ["basecloud", "snow"],     codes: [13, 14, 15, 16, 41, 42, 43, 46] },
    { class: ["basecloud", "hail"],     codes: [17, 35] },
    { class: ["mist"],                  codes: [20, 21, 22] },
    { class: ["basecloud", "frost"],    codes: [25] },
    { class: ["cloud"],                 codes: [26, 27, 29, 33, 28, 30, 34, 44] },
    { class: ["moon"],                  codes: [31] },
    { class: ["sun"],                   codes: [32, 36] },
    // { class: ["none"],               codes: [3200] },
];

// Transform into a lookup table
var WEATHER_CODES = [];
for (var i = 0; i < WEATHER.length; i++) {
    WEATHER[i].codes.forEach(function(code) {
        WEATHER_CODES[code] = WEATHER[i].class;
    });
}

// The original function interface, for compatibility
function returnClass(c) {
    return WEATHER_CODES[c] || ["none"];
}

This is a variant of @tomdemuyt's solution, with two differences:

  • No reliance on JSON hack.
  • returnClass(c) is a simple lookup, for possibly better performance.

If you care about namespace pollution, then you could hide WEATHER and WEATHER_CODES inside a function scope:

var returnClass = (function() {
    var WEATHER = [
        { class: ["basecloud", "wind"],     codes: [0, 19, 23, 24] },
        // etc.
    ];

    var WEATHER_CODES = [];
    for (var i = 0; i < WEATHER.length; i++) {
        WEATHER[i].codes.forEach(function(code) {
            WEATHER_CODES[code] = WEATHER[i].class;
        });
    }

    return function returnClass(c) {
        return WEATHER_CODES[c] || ["none"];
    };
})();
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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