Gfrktyl

Here is my current toggle function:

$scope.toggleSelect = event => {

  $scope.selection = event.target.innerHTML;



  if ($scope.selection === 'Select All') {

     $scope.emailList = $scope.users.filter(user => !user.report.emailed);

  } else {

     $scope.emailList = $scope.emailList = ;

  }

};

Here are my thoughts and questions about the best practices:

1. 
   

   I make an assignment to $scope.emailList in the if and else statement. Instead of separate assignments, is one assignment better? Refactoring to one assignment will pollute the code with unnecessary variables, but it may make the code more readable?

   
   
   

   $scope.toggleSelect = event => {
   
     $scope.selection = event.target.innerHTML;
   
     const emailListUpdate;
   
     if ($scope.selection === 'Select All') {
   
       emailListUpdate = $scope.users.filter(user => !user.report.emailed);
   
     } else {
   
       emailListUpdate = $scope.emailList = ;
   
     }
   
      $scope.emailList = emailListUpdate;
   
   };
   
   

   
   
   

   The main benefit I see here is that anybody reading the code can skim to the last line and know the overall purpose. Overall, I don't see this to be a beneficial refactor since I don't think it adds additional readability and potentially makes it harder to follow. I would appreciate thoughts on this.

   
   


2. 
   

   Ternary or if/else:

   
   
   

   I reviewed a great post about the benefits and use cases of using ternary or if/else. Here is what the code would look like refactored to a ternary:

   
   
   

   $scope.toggleSelect = event => {
   
     $scope.selection = event.target.innerHTML;
   
     $scope.emailList = $scope.selection === 'Select All' ? $scope.users.filter(user => !user.report.emailed); : ;
   
   
   
   };
   
   

   
   
   

   Quoting from the article linked above:

   
   
   

   
   

   An if/else statement emphasizes the branching first and what's to be done is secondary, while a ternary operator emphasizes what's to be done over the selection of the values to do it with.

   
   

   
   
   

   I feel the if/else feels more natural, but I'm not sure if that is just related to my lack of experience using ternary.

   
   


3. 
   

   I have another toggleItem function used when a checkbox is clicked.

   
   
   

   $scope.toggleItem = (event, user) => {
   
     if (event.target.checked) {
   
       $scope.emailList.push(user);
   
     } else {
   
       $scope.emailList = _.reject($scope.emailList, item => item._id === user._id);
   
     }
   
   

   
   

I've thought about combining both of my toggle functions but overall I think it is better to keep them separate instead of trying to make a generic toggle function with multiple use cases.

In the toggleItem function above, I think it is cleaner (and more obvious) to use an if/else instead of a ternary since I am only making an assignment in the else statement.

I'm trying to improve on writing cleaner code, If anyone has any input or thoughts on best practices to use here or how to clean up this code it would be appreciated.

Short and sweet is best.

Your statement

"The main benefit I see here is that anybody reading the code can skim to the last line and know the overall purpose"

That makes no sense at all, you say all that is needed to understand the functions is

$scope.emailList = emailListUpdate;

Nobody jumping into someone else code will just skim, the only people that skim code are those that know the code.

You can make a few assumptions.

• All that read your code are competent coders.


• All that read your code have read the project specs.


• Every line of code will be read by a coder new to the code.

Example

The best code is brief as possible without being a code golf entrant.

Notes

• Why innerHTML, should it not be textContent????
  


• This function is not a toggle. It is based on selection value.


• The ternary expression is too long, break the line so it does need to be scrolled


• The ternary has a syntax error. Misplaced ;
  


• the ; on the last line after "}" is redundant.

Code, best option.

$scope.selectEmailUsers = event => {

  $scope.selection = event.target.textContent;

  $scope.emailList = $scope.selection === "Select All" ? 

       $scope.users.filter(user => !user.report.emailed); : ;

  //                                                    ^ remove syntax error

}; // << remove the ;