Selecting/ Deselecting All from Checkbox

Posted on

Problem

$scope.selectAllProctoringUser = function(checked) {
    try {
        for (var k = 0; k < $scope.scheduleUsers.lstOnlineProctoring.length; k++) {
            $scope.scheduleUsers.lstOnlineProctoring[k].selected = checked;
            if (checked) {
                let flag = 0;
                if($scope.selectedUserListOP.length == 0) {
                    $scope.selectedUserListOP.push($scope.scheduleUsers.lstOnlineProctoring[k]);
                    flag = 1;
                } else {
                    angular.forEach($scope.selectedUserListOP, (el, idx) => {
                        if(el.StudentScheduleUsrID === $scope.scheduleUsers.lstOnlineProctoring[k].StudentScheduleUsrID) {
                            flag = 1;
                        }
                    });
                }
                if(flag === 0){
                    $scope.selectedUserListOP.push($scope.scheduleUsers.lstOnlineProctoring[k]);
                }

            } else {
                angular.forEach($scope.selectedUserListOP, (el, idx) => {
                    if(el.StudentScheduleUsrID === $scope.scheduleUsers.lstOnlineProctoring[k].StudentScheduleUsrID) {
                        $scope.selectedUserListOP.splice(idx, 1);
                    }
                });
            }
        }
    } catch(e) {
                console.log('Method: selectAllProctoringUser:  ' + e.message);
      }
}

I had replaced the above with below snippets and working perfectly

$scope.selectAllProctoringUser = function(checked) {
    try {
        $scope.scheduleUsers.lstOnlineProctoring.forEach(x => {
            x.selected = checked;
            if(checked){
                if($scope.selectedUserListOP.filter(m => m.StudentScheduleUsrID == x.StudentScheduleUsrID).length == 0)
                    $scope.selectedUserListOP.push(x);
            } else if(!checked){
                if($scope.selectedUserListOP.filter(m => m.StudentScheduleUsrID == x.StudentScheduleUsrID).length > 0)
                    $scope.selectedUserListOP.splice(x, 1);
            }
        });
    } catch(e) {
                console.log('Method: selectAllProctoringUser:  ' + e.message);
      }
}

If the above code could still get reduced? or written instead making use of filter operator

Are there any use cases, substituting the above code may cause failure or exceptions?

Expecting more solutions from what that I had initially coded

Solution

I see what looks like a bug. The x variable (which should probably be renamed to something more informative – maybe user?) is a object (with selected and StudentScheduleUsrID properties) which you remove from the array in the upper code with

} else {
    angular.forEach($scope.selectedUserListOP, (el, idx) => {
        if(el.StudentScheduleUsrID === $scope.scheduleUsers.lstOnlineProctoring[k].StudentScheduleUsrID) {
            $scope.selectedUserListOP.splice(idx, 1);
        }
    });
}

splice‘s first argument should be the index of the element you want to start deleting from. That’s fine. But in the lower code, the first argument you pass to splice is the object x, not the index:

$scope.scheduleUsers.lstOnlineProctoring.forEach(x => {
  // ...
  } else if(!checked){
      if($scope.selectedUserListOP.filter(m => m.StudentScheduleUsrID == x.StudentScheduleUsrID).length > 0)
          $scope.selectedUserListOP.splice(x, 1);
  }

That won’t work, because the first argument is not a number. If you want to remove this x from the array there, find its index in the array with findIndex, then pass that number to splice.

const selectedUserIndex = $scope.selectedUserListOP.findIndex(
  selectedUser => selectedUser.StudentScheduleUsrID === user.StudentScheduleUsrID
);
if (selectedUserIndex !== -1) {
  $scope.selectedUserListOP.splice(selectedUserIndex, 1);
}

On a different note: Errors should not arise in synchronous code in most cases. If you have code that sometimes results in an unexpected error, you should fix the code so that the error doesn’t ever occur (as far as you can tell). If you’ve tested the code in various situations (which you should be doing anyway, as a developer) and you can’t think of any which might cause an error to occur, there shouldn’t be any reason to use try/catch, since it’ll just clutter up the code – and besides, you aren’t doing anything with the error other than logging it to the console, but the error will be logged to the console anyway. So, feel free to remove the try/catch.

It can be reasonable to have a try/catch if any of the following are true and you can do something useful with the error (such as send it to your server logs for examination, or gracefully degrade functionality for the user):

  • You’re awaiting an asynchronous operation that might throw (eg, due to a network problem – very common)
  • Synchronous code you’re calling and don’t have control over may throw (pretty unusual)
  • Despite implementing your own tests, the logic is extremely complicated and you’re not sure if there lingering issues (pretty unusual)
  • You need to break out of a deeply nested function with throw and yield control flow back to a much higher calling function (pretty unusual and somewhat of a code smell – better to test values and return when invalid)

None of the above are going on here.


Instead of filtering inside the loop to check to see if any of the elements in the selectedUserListOP have a matching StudentScheduleUsrID, it would be more elegant and less computationally complex to construct a Set of all the StudentScheduleUsrIDs in the selectedUserListOP beforehand. Then, all you need to do inside the loop is check whether that set .has it:

$scope.selectAllProctoringUser = function(checked) {
  const selectedUserStudentIds = new Set(
    $scope.selectedUserListOP.map(
      user => user.StudentScheduleUsrID
    )
  );
  $scope.scheduleUsers.lstOnlineProctoring.forEach(user => {
    user.selected = checked;
    if (checked) {
      if (!selectedUserStudentIds.has(user.StudentScheduleUsrID)) {
        $scope.selectedUserListOP.push(x);
      }
    } else {
      const selectedUserIndex = $scope.selectedUserListOP.findIndex(
        selectedUser => selectedUser.StudentScheduleUsrID === user.StudentScheduleUsrID
      );
      if (selectedUserIndex !== -1) {
        $scope.selectedUserListOP.splice(selectedUserIndex, 1);
      }
    }
  });
}

If you did have to iterate over the array there, instead of if (someArr.filter(...).length == 0), it would be better to

  • Use strict equality with ===, not loose equality (consider using a linter)
  • To check whether every items in an array passes a test (or doesn’t pass a test), use .every or .somedon’t use .filter to construct a new array only to check its length. Eg:

    if ($scope.selectedUserListOP.every(m => m.StudentScheduleUsrID !== x.StudentScheduleUsrID))
    

Leave a Reply

Your email address will not be published. Required fields are marked *