current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I'm unsure about how to simplify this method properly. The method is basically responsible to allocate data.

The response object is a collection of objects. Each object contains an attribute called order and i have to distinguish between visible and invisible elements. Visible elements are represented by positive integers, invisible elements by a negative integers.

Also it is possible to overwrite the default state if the URL contains an parameter (hidden) with a list of hidden element ids. ?hidden=23,432,3,123

_hasParam() returns boolean

_getParam() returns string (separated by comma [23,432,3,123])

contains() returns boolean

prepare: function (response) {

    var visible = [];
    var invisible = [];

    if (_hasParam('hidden')) {

        var hidden = _getParam('hidden').split(',');

        angular.forEach(response, function (item) {
            if (hidden.contains(item.id)) {
                invisible.push(item);
            } else {
                visible.push(item);
            }
        });

    } else {

        angular.forEach(response, function (item) {
            if (item.order === -1) {
                invisible.push(item);
            } else {
                visible.push(item);
            }
        });

    }

    $rootScope.visible = visible;
    $rootScope.invisible = invisible;
}
share|improve this question

1 Answer 1

up vote 2 down vote accepted

The only thing that stands out a bit is the fact that you do 2 very similar angular.forEach() calls. You could extract these functions out and do only 1 call. I would also make -1 a named constant. I would also declare all variables on top.

What you could consider, but I don't advise it, since it reeks too much of CodeGolf, is to simplify the 2 if statements to 1 single push() call

(hidden.contains(item.id)? invisible : visible).push( item );

This piece of code extracts the filter functions and applies my other suggestions:

prepare: function (response) {

    var visible = [],
        invisible = [],
        HIDE = -1,
        hidden, filter;

    if (_hasParam('hidden')) {
        hidden = _getParam('hidden').split(',');
        /* Hide based on the hidden parameter */
        filter = function (item) {
            if (hidden.contains(item.id)) {
                invisible.push(item);
            } else {
                visible.push(item);
            }
        }
    } else {
        filter = function (item) {
        /* Hide based on the order of the item */
            if (item.order === HIDE) {
                invisible.push(item);
            } else {
                visible.push(item);
            }
        }    
    }

    angular.forEach(response, filter);
    $rootScope.visible = visible;
    $rootScope.invisible = invisible;
}
share|improve this answer
    
Thank you very much... :) –  gearsdigital May 13 '14 at 9:45

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.