1
\$\begingroup\$
$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

\$\endgroup\$
2
  • 1
    \$\begingroup\$ What is the purpose of the try/catch? Are you expecting to encounter errors? If so, where? \$\endgroup\$ Commented May 15, 2020 at 9:54
  • \$\begingroup\$ Default way to handle any exceptions. What happens if it was there or not there? Incase if any exception was thrown from any JS how would you identify it or Have you got any idea about how to deal with exceptions @CertainPerformance \$\endgroup\$ Commented May 15, 2020 at 10:33

1 Answer 1

2
\$\begingroup\$

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 .some - don't use .filter to construct a new array only to check its length. Eg:

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.