1
\$\begingroup\$

So I have some Javascript code that resembles the following:

var mylibrary = new (function ()
{

    this._getLibraryObj = function ()
    {
        var newLibraryObj = {};
        var libraryData = window.specifiedLibraryData;
        // Add head librarian details
        // open library
        // etc. etc. turn into an object from data

        return newLibraryObj ;
    };

    this.item = function (item)
    {

        if (this.obj && item in this.obj)
        {
            return this.obj[item];
        }
        else
        {
            this.obj = this._getLibraryObj();
            if (item in this.obj)
            {
                return this.obj[item];
            }
            return null;
        }
    };

    return this;
})();

What annoys me is that this.item(item) function repeats the item in this.obj[item] and I find it a little hard to read. I was hoping for it to not have to run _getLibraryObj too often.

Question: How would you rewrite this.item() to be short and sweet (DRY and readable)?

EDIT:- From alex's post I realised I left out one constraint: If the item isn't in the library object then it might need to _getLibraryObj() again as the data may have changed

Solution:- I ended up using @Alex Nolan's fifth example (with || instead of && I find it easier to read.):

this.item = function(item)
{
    if(!this.obj || !item in this.obj)
    {
        this.obj = this._getLibraryObj();
    }

    return this.obj[item] || null;
}
\$\endgroup\$
1
\$\begingroup\$

Another downfall of your this.item() function is that if item isn't in this.obj, this._getLibraryObj() will be called every time.

Try

this.item = function(item) {
    if (this.obj) {
        return this.obj[item] || null;
    } else {
        this.obj = this._getLibraryObj();
        return this.item(item);
    }
};

Or if you want it even more minimal:

this.item = function(item) {
    if (this.obj) 
        return this.obj[item] || null;

    this.obj = this._getLibraryObj();
    return this.item(item);
};

If you switch up the statements a little bit you get the most readable IMHO:

this.item = function(item) {
    if (!this.obj) 
        this.obj = this._getLibraryObj();

    return this.obj[item] || null;
};

One final way to do it would be to do:

this.item = function(item) {
    this.obj = this.obj || this._getLibraryObj();

    return this.obj[item] || null;
};

EDIT:

In light of your comment about needing to re-call getLibraryObj

this.item = function(item) {
    if (!(this.obj && item in this.obj))
        this.obj = this._getLibraryObj();

    return this.obj[item] || null;
};

or:

this.item = function(item) {
    this.obj = this.obj && item in this.obj ? this.obj : this._getLibraryObj();

    return this.obj[item] || null;
};

or a one-liner:

this.item = function(item) {
    return (this.obj = (this.obj && item in this.obj ? this.obj : this._getLibraryObj()))[item] || null;
};
\$\endgroup\$
  • \$\begingroup\$ Sorry I left one thing out. If the item isn't in the library object then it might need to _getLibraryObj() again as the data may have changed. I do really like that "final way" Nice and simple ... how could I have not thought of it!? \$\endgroup\$ – James Khoury Sep 6 '11 at 4:09
  • \$\begingroup\$ I guess I could put an if(!item in this.obj){ this.obj = this._getLibraryObj();} or use the second last example and change the if to if(!this.obj || !item in this.obj) (just thought of a one liner: return item in (this.obj = this.obj || this._getLibraryObj()) ? this.obj[item] : (this.obj = this._getLibraryObj())[item] || null;) \$\endgroup\$ – James Khoury Sep 6 '11 at 4:23

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.