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've written a function that takes a number and returns a value depending on where that number lies on a pre-defined scale. That's probably a little confusing so I've put the code below and added a JSFiddle (although you might be better off in the dev console).

var getSize = function(number){
        var sizes = {
            100 : 'url_t',
            240 : 'url_s',
            320 : 'url_n',
            500 : 'url_m',
            640 : 'url_z',
            800 : 'url_c',
            1024 : 'url_l',
        },
        result = sizes[100];

    for (var size in sizes){
        if(number < size){
            result = sizes[size];
            break;
        }
        result = sizes[1024];
    }
    return result;
};
getSize(10) // returns url_t
getSize(130) // returns url_s
getSize(2000) // returns url_l
getSize(600) // returns url_z

It works and it's fairly performant, it's just a bit icky. I've tried to come up with a better solution but it just feels ickier. Is there anything I'm missing that would make this a bit nicer? I was playing about with putting the sizes in an array so they're a little more JSON-ish, but then I got lost.

share|improve this question
1  
I would not trust for in to be always properly sorted. –  njzk2 Jul 22 at 20:21
    
I agree, that's one of the reasons I was looking at putting it in an array. Whilst the above code does work, it's certainly not right. –  Sam Beckham Jul 22 at 20:24

5 Answers 5

up vote 12 down vote accepted

You might not need to loop through an object, and a simple switch statement would suffice.

function getSize(number) {
    switch (true) {
        case number < 100: return "url_t";
        case number < 240: return "url_s";
        case number < 320: return "url_n";
        case number < 500: return "url_m";
        case number < 640: return "url_z";
        case number < 800: return "url_c";
        default:           return "url_l";
    }
}

As a side note, I've seem people use switch (true) before when they want to use some logic in their case's instead of just hard coded values. Kind of a nice alternative to an if-else construct, and would probably perform better than looping over an object using a for-in loop.

You could still keep the sizes in an object:

var sizes = {
    100 : 'url_t',
    240 : 'url_s',
    320 : 'url_n',
    500 : 'url_m',
    640 : 'url_z',
    800 : 'url_c',
    1024 : 'url_l'
};

function getSize(number) {
    switch (true) {
        case number < 100: return sizes[100];
        case number < 240: return sizes[240];
        case number < 320: return sizes[320];
        case number < 500: return sizes[500];
        case number < 640: return sizes[640];
        case number < 800: return sizes[800];
        default:           return sizes[1024];
    }
}

Edit: A problem arises if you need to change the sizes for your application. That's why I'm leaning towards the first function, which just a switch statement and hard coded values.

share|improve this answer
    
In this simple example, the first is much more clear. However, if sizes were localized or loaded from some external source, then the second example would necessarily be the only option. –  ssube Jul 22 at 17:11
    
I can't believe I didn't think of switch statements. I generally stay away from them but this is almost a textbook case for them. Thanks Greg. The only issue would be if the sizes were to change, or get passed in dynamically. They don't in this case though so it's all good. –  Sam Beckham Jul 22 at 20:28
  • Your for..in construct should have an Object.hasOwnProperty check in the extremely unlikely event that somebody decides to abuse prototypes (and on Object, no less!).
  • Your indentation is a little inconsistent.
  • You assign to the result variable excessively.
  • Some values are hard-coded, but they shouldn't be. At the very least, move them into a constant instead of leaving them as magic numbers.

You could try something like the following:

var getSize = function(number) {
    var MAX_SIZE = 1024;

    var result;
    var sizes = {
        100: 't',
        240: 's',
        320: 'n',
        500: 'm',
        640: 'z',
        800: 'c',
        1024: 'l'
    };

    for (var size in sizes) {
        if (sizes.hasOwnProperty(size)) {
            if (number < size) {
                result = sizes[size];
                break;
            }
        }
    }
    return "url_" + (result || sizes[MAX_SIZE]);
}

Incidentally, is it intentional that getSize(100) will return url_s instead of url_t? If not, just change the < above to a <=.


Revision. Better yet, we can use Object.keys(). This has the advantage of being entirely unreliant on magic numbers; we can instead just change our sizeMap object as needed. Furthermore, this deals with @megawac's concern about the undefined enumeration of for...in.

var getSize = function(number) {
    var result;
    var sizeMap = {
        100: 't',
        240: 's',
        320: 'n',
        500: 'm',
        640: 'z',
        800: 'c',
        1024: 'l'
    };

    var sizes = Object.keys(sizeMap).sort(function(a, b) {
        return a - b;
    });

    for (var i = 0; i < sizes.length; i++) {
        if (number < sizes[i]) {
            result = sizeMap[sizes[i]];
            break;
        }
    }
    return "url_" + (result || sizeMap[sizes[sizes.length - 1]]);
}
share|improve this answer
1  
Instead of result || sizeMap[sizes[sizes.length - 1]] you can set result = sizeMap[..] before the if and return result –  megawac Jul 22 at 16:54

Alright first thing the order that an engine enumerates object properties is not guaranteed!!! Its pretty consistent but there are engine bugs and you shouldn't rely on the behaviour.

That said, this is how I would write your getSize. You can use Object.keys(sizes) to create the ranges list but again - order isn't gaurenteed. You could sort it I suppose?

var getSize = (function() {
    var sizes = {
        100 : 'url_t',
        240 : 'url_s',
        320 : 'url_n',
        500 : 'url_m',
        640 : 'url_z',
        800 : 'url_c',
        1024 : 'url_l',
    };
    // Note you can do Object.key(sizes); to create this list
    var ranges = [100, 240, 320, 500, 640, 800, 1024];

    return function(number) {
        for (var i = 0, len = ranges.length; i < len; i++) {
            var key = ranges[i];
            if (number < key) {
                return sizes[key];
            }
        }
        // otherwise return the largest size
        return sizes[key];
    };
})();

It may be overkill but I also created the sizes and ranges object once

share|improve this answer
    
+1 for undefined enumeration order, was the first thing that jumped out at me (and the other answers seem to have missed it) –  Dagg Jul 22 at 19:41
    
Spot on with the enumeration order. I was made aware of this when I was writing it but I couldn't think of a nice way around it. Declaring the ranges is a good way around it but wouldn't the enumeration order issue still crop up if I created it with Object.key(sizes)? –  Sam Beckham Jul 22 at 20:37
    
@SamBeckham it depends on what environments you want to support. If you take a look at the chrome ticket theres a lot of talk about standardizing the order, in which case it will no longer be an issue. AFAIK all evergreen browsers will iterate numerical props in numeric order but some legacy will iterate in the order added –  megawac Jul 22 at 20:49
1  
I would advise against relying on a feature which is explicitly undefined –  megawac Jul 22 at 20:49

I'd go with a variation on Greg's excellent answer, but I would invert the sizes map to eliminate magic numbers:

var sizes = {
    url_t : 100,
    url_s : 240,
    url_n : 320,
    url_m : 500,
    url_z : 640,
    url_c : 800,
    url_l : 1024
};

function getSize(number) {
    switch (true) {
        case number < sizes.url_t: return 'url_t';
        case number < sizes.url_s: return 'url_s';
        case number < sizes.url_n: return 'url_n';
        case number < sizes.url_m: return 'url_m';
        case number < sizes.url_z: return 'url_z';
        case number < sizes.url_c: return 'url_c';
        default:           return 'url_l';
    }
}

Or, instead of the switch, to avoid the repetition of its case lines:

var names = ['url_t', 'url_s', 'url_n', 'url_m', 'url_z', 'url_c', 'url_l'];

function getSize(number) {
    for (var i in names) {
        var name = names[i];
        if (number < sizes[name]) {
            return name;
        }
    }
    return names[names.length - 1];
}

And another thing, keep in mind that a , right before the closing } is incorrect JavaScript and may not work in all browsers and environments:

// incorrect:
var sizes = { 1024 : 'url_l', }

// correct:
var sizes = { 1024 : 'url_l' }
share|improve this answer
    
Yeah, I always forget about that trailing comma. It's JSHint's favourite thing to catch me out on. I prefer the switch without the sizes map to be honest. It seems like I'm basically repeating the same map inside the switch statement anyway. Thanks for all the help on this. –  Sam Beckham Jul 22 at 20:42

I won't use the for in structure.

It's loops thorught the properties of the object and also the properties of his prototype.

If that's your only choice you have to use the hasOwnProperty for every size you check to avoid posible errors.

I would use something like:

var sizes = {
 values: [100,240,320,500,640,800,1024],
 names: ['url_t', 'url_s', 'url_n', 'url_m', 'url_z', 'url_c', 'url_l']
},

and then the usual for loop.

share|improve this answer
1  
I actually think for..in is completely valid here. It makes more sense than having to update two arrays... –  Schism Jul 22 at 15:34
    
I just feel more comfortable using arrays to save data, and objects for complex structures, but yeah, the two arrays solution will probably end wrong –  david ruiz Jul 22 at 15:36
    
Yeah, I've heard that about for in. I'm curious though, how could any properties be added to the sizes object in this case? –  Sam Beckham Jul 22 at 20:48

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.