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

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've been working on an app based on the MEAN Framework. I have got the app working but I just wasnt sure whether my coding structure and standard was good enough, meaning does it adhere to the programming standards in terms of structure, security etc.

So I'll explain my app:

A very simple app, it consists of a MongoDB database which contains a large amount of projects and its details such as results, costs, outcome etc.

My app serves as a interface which provides a in-depth filter function. Very similar to what eBay does to filter their products. You choose different filter options like Project start/end date, Project Cost, Project Type, Project results and that filters the projects down to what matches the users criteria.

Note that the once the user selects the filter options they send a request to my API (node.js) and my server then filters the results server side and sends back the filtered data to be displayed on the front-end.

And thats it!

My Coding Structure:

My coding structure was:

  1. All functions that call to my API/Server or handle sensitive data were added into a angularjs Factory.
  2. My controllers were only responsible for gathering data and sending it to my factory. AND receiving data back from the factory to update my view.
  3. My view is based on "EJS" tempting engine & uses AngularJS heavily.
  4. The actual sorting/filtering/manipulating of data is ALL done Server side!

My Code:

Controller:

// This is our controller for the knowledgebase page
  angular.module('knowledgeBaseCtrl', ['ngAnimate'])

  .controller('projectsFilterCtrl', function($scope, $http, projectsFilterService){

   $scope.getProjects = function(){ 
    //The filtered objected that contains the users filteration options and is passed as a parameter to the server
    var filterObj = {};

    var regionsArr = [];
    var servicesArr = [];

    //Getting users filtertion options  
    var form = document.getElementById("regionPicker");
    var servicesForm = document.getElementById("servicesPicker");

    var sectorForm = document.getElementById("selectSector");
    var sector = sectorForm.options[sectorForm.selectedIndex].value;

    var subSectorForm = document.getElementById("selectSubSector");
    var subSector = subSectorForm.options[subSectorForm.selectedIndex].value;

    var clientForm = document.getElementById("selectClient");
    var client = clientForm.options[clientForm.selectedIndex].value;

    var inputs = form.getElementsByTagName("input");
    var servicesInput = servicesForm.getElementsByTagName("input");

    // Forming user options into arrays
    for (var i = 0;  i < inputs.length; i += 1) {
         // Take only those inputs which are checkbox
        if (inputs[i].type === "checkbox" && inputs[i].checked) {
                 regionsArr.push(inputs[i].value);
        }
    }

    for (var i = 0;  i < servicesInput.length; i += 1) {
         // Take only those inputs which are checkbox
        if (servicesInput[i].type === "checkbox" && servicesInput[i].checked) {
                 servicesArr.push(servicesInput[i].value);
        }
    }

    // Adding the key/value pairs to the filter object
    filterObj.region = (regionsArr.length > 0) ? regionsArr:"";
    filterObj.sol = (servicesArr.length > 0) ? servicesArr:"";
    filterObj.sector = (sector.length <= 0) ? "":sector;
    filterObj.subSector = (subSector.length <= 0) ? "":subSector;
    filterObj.client = (client.length <= 0) ? "":client;


    // testing that data is indeed being added to the array
    console.log("Regions selected " + regionsArr);
    console.log("Services selected " + servicesArr);
    console.log("FILTER OBJ " + JSON.stringify(filterObj));
    console.log("TEST" + filterObj.sol.length);

   // Conditional statement to call the filter service if any of the variables has a value    
   if(regionsArr.length > 0 || servicesArr.length > 0 || sector.length > 0 || subSector.length > 0 || client.length > 0){

   //calling a function from the AngularJS Factory to get the projects from the database
   projectsFilterService.getFilteredProjects(filterObj)
       .then(
           function(result) {
               $scope.showFilterAlert = false;
               $scope.myData = result;

               // Show the projects div if the response is succesfull
               $scope.showNow = true;
               console.log("Sucessfully getting " + result);
           },
           function(error) {
               console.log(error.statusText);
           }
       );
    }else {
        $scope.showFilterAlert = true;

    }
};

// Function to clear out the filtration options menu and reset to all nill values
$scope.clearFilter = function(){

   $scope.showNow = false;

   document.getElementById('sectorPicker').reset();
   document.getElementById('subSectorPicker').reset();
   document.getElementById('clientPicker').reset();

   $(":checkbox").prop('checked', false).parent().removeClass('active');

    console.log("filters cleared succesfully");
};
});

Factory:

// This is our service or factory for the knowledge base page, responsible for connecting to the API/Server
angular.module('knowledgeBaseFilterService', [])

.factory('projectsFilterService', function($http, $q){

var deferred = $q.defer();

var factory = {};

factory.getFilteredProjects = function(params){
    //console.log("HSGDJHAS" + JSON.stringify(params));
  return $http.post('/user/test',params).
    success(function(data, status, headers, config) {
        // this callback will be called asynchronously
        // when the response is available
        console.log("this is the response data " + data.length);
    }).
    error(function(data, status, headers, config) {
        // called asynchronously if an error occurs
        // or server returns response with an error status.
    });
};

  return factory;
});

My Issues:

The things that worry me are:

  1. I am using a lot of "getElementById" in my controller, is that a good thing?
  2. My controller contains a lot of variables/arrays. For each "getElementById" I had to create a variable. Is that a good thing?
  3. The reason I used a factory was because it allowed me to keep my functions out of the global namespace. Is there any variables or functions you think are in the global namespace that could be bad or just bad coding practice?

So my ultimate question is that, by looking at my code above and looking at how I structured it, Does it adhere or conform good programming practices or more precisely good JavaScript practices? Is there anything I should change?

I am looking for a way to improve my code and myself as a programmer.

share|improve this question

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.