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
await
ing 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 StudentScheduleUsrID
s 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))