3
\$\begingroup\$

I've been working on a lightweight cookiemanager and version 1 has been completed and works perfectly as intended. I would love to get a review to see where we can improve it.

Where should I throw logical errors? For now I only log.

Its usage is as follows:

Getting cookies can be done by calling Cookiemanger.get(). You can supply a name argument to get a specific cookie, if no name is supplied it will return all cookies currently in the document. An additional flag can be set as a second argument whether to use strict matching on the cookies name.

Cookiemanager.get(); // returns all cookies currently defined in the document as a name/value object.
Cookiemanager.get('your_cookie_name'); // returns cookie(s) as a name/value object, using loose matching. so 'your_cookie_name_123' will get returned also.
Cookiemanager.get('your_cookie_name', true); // Same as above only now it will do strict name matching.

Setting cookies can be done by calling Cookiemanger.set(). It requires a settings object with at least a name key/value supplied.

// Simple setting
Cookiemanger.set({
    name : 'your_cookie_name',
    value : 'your value'
});

// Extended setting
Cookiemanger.set({
    name : 'your_cookie_name',
    value : 'your value',
    expires : 'a expiry date in as a UTC string'
    path : '/',
    domain : document.location.host,
    secure : false
});

Removing cookies can be done by calling Cookiemager.remove(). It accepts 3 arguments: name, domain and path as a string. domain and path are optional. If none are supplied, it will remove all cookies from the domain including all sub paths.

// Removes the cookie at the current domain, including all sub paths.
Cookiemanger.remove({
     name : 'your_cookie_name',
     domain : document.location.host
});

// Removes the cookie at the supplied path, excluding sub paths.
Cookiemanger.remove({
     name : 'your_cookie_name',
     path: '/'
});

Testing a cookie against a certain value can be done by calling Cookiemanger.test(). It accepts a settings object as below with at least a name supplied. You can add custom values if a value matches with the value in the current cookie and it will call the function if supplied. If no cookies are found with the name supplied you can pass a undefined, as seen below.

Cookiemanger.test({
    name : 'testcookie',
    'an_expected_value' : function () {
    },
    'undefined' : function () {
    }
});

;var Cookiemanager = (function (Cookiemanager, window, document, undefined) {

'use strict';

function isObject (arg) {
    return Object.prototype.toString.call(arg) === '[object Object]';
};

function isArray (arg) {
    return Object.prototype.toString.call(arg) === '[object Array]';
};

function isString (arg) {
    return Object.prototype.toString.call(arg) === '[object String]';
};

function isEmptyObj (argument){
    for( var key in argument ) return false;
    return true;
};

function returnCookieString (sName, sValue, sExpires, sPath, sDomain, bSecure) {
    var sCookie = (sName + '=' + escape(typeof sValue !== 'undefined' ? sValue : '')) + 
    (isString(sExpires) && sExpires.indexOf('GMT') !== -1 ? ';expires=' + sExpires : '') + 
    (isString(sPath) ? ';path=' + sPath : ';path=') + 
    (isString(sDomain) ? ';domain=' + sDomain : '') + 
    (bSecure ? ';secure' : '');
    return sCookie;
};

function returnURIPaths () {
    var i,
    sURIPath,
    aURIPaths = [],
    aURISegments = document.location.pathname.replace(/\/$/, '').split('/'),
    iNrOfURISegments = aURISegments.length;

    for (i = iNrOfURISegments; i--;) {
        sURIPath = aURISegments.slice(0, i + 1).join('/');
        aURIPaths.push(sURIPath);
        aURIPaths.push(sURIPath + '/');
    }

    return aURIPaths;
};

function error (msg) {
    throw new Error(msg);
};

function log (msg) {
    try {
        console.log(msg);
    } catch (e) {}
};

Cookiemanager.get = function (arg, strict) {
    var sCookies = document.cookie;
    if (sCookies.length) {
        var i, 
        oResult = {},
        aCookies = sCookies.split(';'),
        iNrOfCookies = aCookies.length,
        sCookie,
        aCookieValues,
        sCookieName,
        sCookieValue;
        for (i = iNrOfCookies; i--;) {
            sCookie = aCookies[i];
            if (sCookie.charAt(0) === ' ') {
                sCookie = sCookie.substring(1, sCookie.length);
            }
            aCookieValues = sCookie.split('=');
            sCookieName = aCookieValues[0];
            sCookieValue = unescape(aCookieValues[1]);
            if (arg && isString(arg)) {
                if (strict) {
                    if (sCookieName === arg) {
                        oResult[sCookieName] = sCookieValue;
                    } 
                } else {
                    if (sCookieName.indexOf(arg) !== -1) {
                        oResult[sCookieName] = sCookieValue;
                    } 
                }
            } else {
                oResult[sCookieName] = sCookieValue;
            }
        }
        if (! isEmptyObj(oResult)) {
            return oResult;
        } else {
            log('Cookiemanager.get(): Could not find cookie with supplied name.');
        }
    } else {
        log('Cookiemanager.get(): No cookies defined in the current document.');
    }
};

Cookiemanager.set = function (arg) {
    if (arg && isObject(arg)) {
        if (isString(arg.name)) {
            document.cookie = returnCookieString(arg.name, arg.value, arg.expires, arg.path, arg.domain, arg.secure);
        } else {
            log('Cookiemanager.set(): Cookie not set, please provide a valid name.');
        }
    } else {
        log('Cookiemanager.set(): Cookie not set, please provide a valid settings object.');
    }
};

Cookiemanager.remove = function (sName, sPath, sDomain) {
    if (isString(sName)) {

        var expires = new Date();
        expires.setFullYear(expires.getFullYear() - 1);

        if (isString(sPath)) {
            this.set({ 
                name    : sName,
                expires : expires.toUTCString(),
                path    : sPath,
                domain  : typeof sDomain !== 'undefined' ? sDomain : document.location.host
            });
        } else {
            var i,
            aURIPaths = returnURIPaths(),
            iNrOfURIPaths = aURIPaths.length;
            for (i = iNrOfURIPaths; i--;) {
                this.set({ 
                    name    : sName,
                    expires : expires.toUTCString(),
                    path    : aURIPaths[i],
                    domain  : typeof sDomain !== 'undefined' ? sDomain : document.location.host
                });
            }
        }
    } else {
        log('Cookiemanager.remove(): No cookies removed, please provide a valid name.');
    }
};

Cookiemanager.test = function (arg, strict) {
    if (arg && isObject(arg)) {
        var sName = arg.name;
        if (isString(sName)) {
            var oCookie = this.get(sName, strict || false);
            if (isEmptyObj(oCookie)) {
                arg['undefined'].call(this);
            } else {
                for(var key in arg) {
                    if (key === oCookie[sName]) {
                        arg[key].call(this);
                    }
                }
            }
        } else {
            log('Cookiemanager.test(): Could not test, please provide a valid name.');
        }
    } else {
        log('Cookiemanager.test(): Could not test, please provide a valid settings object.');
    }
};

return Cookiemanager;

}(Cookiemanager || {}, this, this.document));
\$\endgroup\$
4
\$\begingroup\$

For a "lightweight" cookie manager, there seems to be rather a lot going on in my opinion. And I'd call the object CookieManager since it's two words, but that's not structurally/syntactically important.

I am however a bit suspicious of the get() usage. If I give it a string, and nothing else, I'd expect that to do strict name-matching. I.e. given a name, I expect zero or 1 cookie in return, not a list. Maybe break out the "fuzzy matching" into a find() method, or have get() accept a regular expression instead.

Speaking of regular expressions, that's probably the easiest way to parse the document.cookie string:

function all() {
  var cookies = {};
  // using replace, as it accepts a function
  document.cookie.replace(/([^=]+)=([^;]*)[;\s]*/g, function(match, name, value) {
    cookies[name] = value;
  });
  return cookies;
}

That'll get you an object (or hash or map if you prefer) of all the cookies. You might also expose that as CookieManager.all() instead of overloading the get() method with yet another usage.

Internally, get() can use all() to get the individual cookies, and basically become a very simply shortcut

function get(name) {
  return all()[name];
}

I wouldn't bother logging errors within the cookie manager, as you'll still have to check the return value elsewhere. In this case, get() will simply return a string value or undefined. GIGO: Garbage in, garbage out.

Now, for fuzzy name-matching, I'd add a find() function to have something distinct from the stricter get() function. Something like

function find(pattern) {
  var name,
      found = {},
      cookies = all();
  if( pattern instanceof RegExp ) {
    for( name in cookies ) {
      if( pattern.test(name) ) {
        found[name] = cookies[name];
      }
    }
  } else {
    for( name in cookies ) {
      if( name.indexOf(pattern) !== -1 ) {
        found[name] = cookies[name];
      }
    }
  }
  return found;
}

Once again, GIGO. Not necessarily your responsibility to check the arguments.

Lastly, I'd probably have set() accept specific arguments, rather than an object simply because it makes its usage more obvious. If I just have to pass an object, I still have to know what that object should contain.
Another reason would be to keep it consistent with the remove() function, which takes specific arguments already.

set() and remove() are the two functions that should probably do some input-checking and maybe throw an exception. This is mostly as they can't really follow the GIGO approach, because they don't return something. So it's more like "garbage in, throw exception"
Again, not much reason to attempt to log, just to risk throwing an exception there; probably easier to just throw if the input is spurious.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the response, very constructive! I haven't thought of the regular expression approach, but looking at it i'll definitely incorporate it, although regex ain't my strongest point it will be a good learn. What would you define as 'lightweight'? I'll be throwing this script on github soon so everyone can contribute if willing. \$\endgroup\$ – ngr Mar 30 '13 at 17:54
  • \$\begingroup\$ @ngr Well, in terms of functionality (get, set, remove) your code is pretty "lightweight". I was mostly looking at it and felt like there was too much code for those 3 simple operations. A lot of it has to do with the (in my opinion unnecessary) error logging and type checking. Type checking is sometimes useful of course, but again: GIGO for the most part. \$\endgroup\$ – Flambino Mar 30 '13 at 20:24
  • \$\begingroup\$ I am currently refactoring with your above suggestions and it cleans the methods up really nice, also it follows the single responsibility principle more then before. The regular expression i find still hard to grasp, but then again i think that's an art on its own. :) Merci! \$\endgroup\$ – ngr Mar 30 '13 at 21:12
  • \$\begingroup\$ With regard to the domain flag, isn't it useful for cookies on a specific subdomain? For example setting and removing cookies on sub.domain.com \$\endgroup\$ – ngr Mar 30 '13 at 21:45
  • \$\begingroup\$ @ngr Sorry, yes, you're right about the domain part. I don't know what I was thinking. By the way, if you think the answer helped, click the checkmark next to it. Thanks \$\endgroup\$ – Flambino Mar 30 '13 at 22:41

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.