Home How to refactor my angularjs function with if validations and 2 foreach loops
Reply: 2

How to refactor my angularjs function with if validations and 2 foreach loops

zomdar
1#
zomdar Published in 2017-11-14 16:13:48Z

I've written this function in order to loop through some files and their children items depending on certain criteria. Depending on what the criteria is for the file, it will return true or false.

I've made a variable validFile to keep track of the return value.

I feel like there is a cleaner way to make the function more fluid....if someone can give me some pointers on how i can make this function more simple that'd be great.

function checkFile() {
    // track file validity value
    var validFile;

    // check to see if files exists
    if($scope.files.length) {
      angular.forEach($scope.files, function(file) {
         // check to see that file has a child item
         if(file.item && file.item.child.length > 1) {
          angular.forEach(file.item.child, function(item) { 
            if(item.child.code === 8) {
              validFile = true;
            }
          });
        } else {
          validFile = false;
        }

      });
    } else {
      validFile = false;
    }

    return validFile;
  }

PS. the function works fine....just want to clean it up and refactor it to be neater

EDIT:

example of files goes something like this...

 $scope.files = [{
      length: 1,
      type: pdf,
      item: {
        child:[{
           type: pdf,
           code: 8
        }]
      }
    }]

The validFile will be true if

  • $scope.files and file.item.child exists

  • item.child.code has a value of 8

thanks

dhilt
2#
dhilt Reply to 2017-11-14 17:16:30Z

I would do the following refactoring:

  function checkFile() {
    var result = false;
    if(angular.isArray($scope.files)) {
      angular.forEach($scope.files, function(file) {
         if(file.item && angular.isArray(file.item.child)) {
          angular.forEach(file.item.child, function(child) { 
            if(child.code === 8) {
              result = true;
            }
          });
        }
      });
    }
    return result;
  }

Main things that I did:

  • checking for an array of top-level object is switched to angular.isArray;
  • file.item.child.length could be 1;
  • inner loop is protected by angular.isArray condition;
  • inner loop iteration variable is child, not item;
  • also I generalized the result flag with false as initial value.

Furhter refactoring could contain following ideas:

  • replace forEach logic with breakabe logic (plain old for-loop or Array.prototype.some or something else) to reduce iterations amount...
inorganik
3#
inorganik Reply to 2017-11-14 16:57:56Z

The function name is a bit of a misnomer, since you're checking files not a single file. Break it out then call your function in a loop.

You can also remove a few lines by just initially setting validFile to false.

function checkFile(file) {
  var validFile = false;
  if(file.item && file.item.child.length > 1) {
    angular.forEach(file.item.child, function(item) { 
      if(item.child.code === 8) {
        validFile = true;
      }
    });
  }
  return validFile;
}

angular.forEach($scope.files, checkFile); // ok to have length of 0
You need to login account before you can post.

About| Privacy statement| Terms of Service| Advertising| Contact us| Help| Sitemap|
Processed in 0.32107 second(s) , Gzip On .

© 2016 Powered by mzan.com design MATCHINFO