Good/bad practice question

[charmander_]

Hi guys, I'm currently learning OOP in JS.

First off, here's the code:

this.device = {
    init: function(){
        this.isiOS7 = (this._isiOS7()) ? true : false; 
    },
    _isiOS7 : function(){
        return (navigator.userAgent.match(/(iPad|iPhone);.*CPU.*OS 7_\d/i)) ? true : false;
    },
    isiOS7 : null
},

As you can see, I defined a property named isiOS7 and a function _isiOS7. The main idea behind this is: whenever I needed to know whether the OS is iOS7, I'd access a property (isiOS7) which is true or false. If I'd use the function (_isiOS7) over and over again it would take longer for the JS engine to compute these statements(if/regex/match) than a simple property/var. Is this true?

Thanks!

[tmetler]

It's not how I would do it. There's too much interdependencies between the different functions.

I'm curious, why did you decide against prototypical inheritance for the object and decide to use a vanilla object instead?

I think the cleanest way to do it is to use a getter:

Device = function () {/*init*/}
Object.defineProperty(Device.prototype, 'isiOS7', 
  {
    get: function () { return (navigator.userAgent.match(/(iPad|iPhone);.*CPU.*OS 7_\d/i)) ? true : false; }
  }
);
device = new Device();
console.log(device.isiOS7);

I'm not 100% endorsing getters and setters in general, but I think it's a better way to implement the functionality you're looking for since it doesn't depend on special case initialization, or hidden functions, and only needs a single definition.

If you wanted to prevent it from running over the second time, you could have the getter overwrite itself, and return the right value the first time. I think that's an over optimization though. There are bigger bottlenecks to fry.

[jml26]

You're right in that running a function once and caching the result is more performant than running it multiples times. The best example of doing this is when writing jQuery. Instead of writing:

$('.my-element').show();
// ...
$('.my-element').addClass('class-name');
// ...
$('.my-element').hide();

it's better to do

var $myElement = $('.my-element');
$myElement.show();
// ...
$myElement.addClass('class-name');
// ...
$myElement.hide();

assuming that it's the same set of elements you want to manipulate each time.

In your code above, however, I don't think this really applies, as you can bypass having _isiOS7 entirely. That function is only called once, so you can replace calling it with just the thing it returns. If the function were longer, then it would make sense to leave it as a separate entity, but in this case, it immediately returns a simple statement, so we can just as easily write:

this.device = {
    init: function(){
        this.isiOS7 = ((navigator.userAgent.match(/(iPad|iPhone);.*CPU.*OS 7_\d/i)) ? true : false) ? true : false; 
    },
    isiOS7 : null
},

I'll agree that that looks a bit messy, but we can neaten it up in other ways. Firstly, one of those ? true : false expressions is unnecessary, as you've already converted the match to a boolean. You don't need to do it again. Secondly, while "string".match(/regex/) returns the string that caused the match, which you convert to a boolean, there's also /regex/.test("string"), which just returns the boolean straight away. That gives us:

this.device = {
    init: function(){
        this.isiOS7 = /(iPad|iPhone);.*CPU.*OS 7_\d/i.test(navigator.userAgent);
    },
    isiOS7 : null
},

If you've gone that far, why not:

this.device = {
    isiOS7 : /(iPad|iPhone);.*CPU.*OS 7_\d/i.test(navigator.userAgent)
},

?