1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Second snippet second last line should not be there.... \$\endgroup\$ Commented Mar 23, 2019 at 14:32
  • \$\begingroup\$ Please provide some context for this code. What exactly does it do? Is it Angular? If so, please add a tag and add some details. See How to Ask. \$\endgroup\$ Commented Mar 23, 2019 at 14:47

2 Answers 2

Reset to default
5
\$\begingroup\$

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

Some comments here are inspired by below line:

Programs must be written for people to read, and only incidentally for machines to execute. — Abelson and Sussman

  • When I first looked at the code, first thing I could not stop appreciating was avoiding the ternary operator. Using if makes it easier to setup correct expectations on what is happening without extra attention.

  • 'Select All' is a CONSTANT which is deciding the method output. So, my suggestion is to move it to the top of the method and assign to a variable.

  • When the code contains conditional execution, having a single place that consumes the result of different blocks would be good choice. I see in your second block of code you made that change. $scope.emailList = emailListUpdate;

  • $scope.emailList = _.reject($scope.emailList, item => item._id === user._id); I review lot of code internally. No way I would accept code like this. It's not about how smartly we can write code, it's about how easy it is for others to understand. Separate the logic from return statements. Give it a meaningful name to convey what it is and why are you filtering.

  • $scope.users.filter(user => !user.report.emailed) is smart! But $scope.users.filter((user) => { return user.report.emailed === false }) is easier to understand what's happening.

Hope this helps!

Edit: I am curious to understand why my answer deserve negative vote. Someone please take a minute to help me understand.

\$\endgroup\$
5
  • \$\begingroup\$ Where ever you got that quote from, it is absurd "only incidentally... to execute" 😲. First Programs are not source code. Second, the few people that can read source code have specialized knowledge, training and experience related to the programs application Third source codes PRIMARY purpose is to be machine readable, without ambiguity, as such to be, interpreted / compiled / transpiled to a consistently executable form for targeted platforms in a sustainable efficient manner without error or bug \$\endgroup\$ Commented Mar 24, 2019 at 12:23
  • 4
    \$\begingroup\$ @Blindman67 the quote, as noted, is from the authors of SICP; if you have to argue, argue with them. \$\endgroup\$ Commented Mar 24, 2019 at 18:33
  • \$\begingroup\$ @morbusg Thank you for adding the reference. \$\endgroup\$ Commented Mar 25, 2019 at 0:22
  • \$\begingroup\$ At a guess console.log("Hello World") with 8 redundant lines (I counted 9 ;). Re your edit Guessing I would think you got a down vote for the last point. The two expressions you give are not equivalent v => !v not the same as v => v === false. Personally I am temped to down vote due to the quote and the 2nd last point, The 2nd point I have read it three times and still not sure why you find the expression unacceptable and you offer no alternative. However your a newbie here so will say Hi welcome to CR, I am looking forward to your next post. 👍 \$\endgroup\$ Commented Mar 25, 2019 at 18:06
  • 1
    \$\begingroup\$ @Blindman67 Thanks, I had few learnings discussing with you. BTW, I am a newbie to CR but not to JavaScript. Glad to hear you that you will follow my answers, I can assure you I will not try chasing others answers with my opinions. Thanks for welcoming to CR. I am sure I will have wonderful time trying to help people write code with mindfulness based on my experiences. \$\endgroup\$ Commented Mar 26, 2019 at 5:46

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.