Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have an AngularJS function that is used to determine which $scope associative array variable is to have data pushed into it. I would love to be able to remove the switch case entirely if possible but haven't been able to envision a way to do that! My overall goal is to both improve performance, but more importantly reduce the overall number of lines if possible

I also considered perhaps some type of variable interpolation, but that seemed like an investment with little return on my effort.

The ng-change directive is called in my HTML like this:

<div class="input-group">
            <span class="input-group-addon" id="category-select-addon" style="width:110px">Category:</span>
            <select 
                    class="form-control" 
                    type="select" 
                    name="categories" 
                    ng-model="categorySelect" 
                    aria-describedby="category-select-addon" 
                    ng-options="category.name as category.name for category in categories"
                    ng-change="updateSelection(categorySelect, 'category')" required>
            </select>
        </div>

AngularJS Code:

 $scope.updateSelection = function(value, field) {
    $log.debug("Inside updateSelection");
    $log.info("Values passed in: (" + value + ", " + field + ")");
    switch (field) {
        case 'tool':
            $scope.tool = value;
            $scope.data = $.param({
                table: $scope.tool,
                tool : $scope.tool,
            });

            $scope.dbQuery($scope.url, $scope.data, $scope.config, $scope.type['category']);
            break;

        case 'category':
            $scope.category = value;
            $scope.data = $.param({
                table: $scope.tool,
                tool : $scope.tool,
                category: $scope.category
            });

            $scope.dbQuery($scope.url, $scope.data, $scope.config, $scope.type['subCategory']);
            break;

        case 'subcategory':
            $scope.subCategory = value;
            $scope.data = $.param({
                table: $scope.tool,
                tool : $scope.tool,
                category: $scope.category,
                sub_category: $scope.subCategory
            });
            $scope.dbQuery($scope.url, $scope.data, $scope.config, $scope.type['issue']);
            break;

        case 'issue':
            $scope.issue = value;
            $scope.data = $.param({
                table: $scope.tool,
                tool : $scope.tool,
                category: $scope.category,
                sub_category: $scope.subCategory,
                issue: $scope.issue
            });

            $scope.dbQuery($scope.url, $scope.data, $scope.config, $scope.type['script']);

            $scope.data = $.param({
                table: $scope.tool,
                tool : $scope.tool,
                category: $scope.category,
                sub_category: $scope.subCategory,
                issue: $scope.issue,
                flag: true
            });

            $scope.dbQuery($scope.url, $scope.data, $scope.config, $scope.type['workInstruction']);
            break;

        default:
            $log.error("Error case reached in dbQuery()! Couldn't find a query for case: " + field);
            break;
    }

}

$scope.dbQuery = function dbQuery(url, data, config, member) {
    $http.post(url, data, config)
        .success(function (data, status, headers, config) {            
            var i = 0;
            switch (member) {
                case 'tool':
                    for (i = 0; i < data.length; i++) {
                        $scope.tools.push({name: data[i]});
                    }
                    break;

                case 'category':
                    for (i = 0; i < data.length; i++) {
                        $scope.categories.push({name: data[i]});    
                    }
                    break;

                case 'subCategory':
                    for (i = 0; i < data.length; i++) {
                        $scope.subcategories.push({name: data[i]});    
                    }
                    break;

                case 'issue':
                    for (i = 0; i < data.length; i++) {
                        $scope.issues.push({name: data[i]});    
                    }
                    break;

                case 'script':
                    $scope.script = data[0];
                    break;

                case 'workInstruction':
                    $scope.workInstruction = data[0];
                    break;

                default:
                    break;
            }
        })
        .error(function(data, status, headers, config) {
            var result = 'Bad query: ' + data +
                "<hr />status: "  + status +
                "<hr />headers: " + header +
                "<hr />config: "  + config;
            $log.error(result);
        });
}
share|improve this question

1 Answer 1

There is a lot of repetition in the code.

$scope.tool = value; and similar after every switch can be replaced by single $scope[field]=value;

Every time a data object is created, which deserves to be outsourced in a dedicated service. So is the server request - NO $http inside a controller. Controller's responsibility is only to glue the data from services, not to do more.

function dbQuery(url, data, config, member) has way too many argument, a single object argument makes it more readable.

Also usage controller as is preferable.

share|improve this answer
    
HI Dimitri, thank you for the feedback! I have never heard of putting the $http outside the controller, could you please provide examples of the changes you are suggesting so that I can visualize what you are describing? Also, I tend to disagree that 4 arguments is too many; their use is very easy to read and straightforward. – Evan Bechtol Nov 4 at 1:39
    
@EvanBechtol Search for Angular best practices and style guides, many examples there. Even 2 arguments make function hard to use because you have to remember the order. Bob Martin advocates 0 or at most 1 argument. What appears easy can become unmanageable as application grows. Do as it works for you but at your own risk :) – Dmitri Zaitsev 2 days ago
1  
@EvanBechtol Check this Angular styleguide for where to put server requests: github.com/gocardless/… – Dmitri Zaitsev 2 days ago

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.