0
\$\begingroup\$

I have the following if statement:

if ($scope.current < $scope.questions.length) {
    $scope.current += 1;
}

if ($scope.current == $scope.questions.length &&
    $scope.chapter < $scope.all.length - 1) {

    $scope.current = 0;
    $scope.chapter += 1;
}            


if ($scope.current == $scope.questions.length &&
    $scope.chapter == $scope.all.length - 1) {

    $scope.current -= 1;
    $scope.done = true;

    console.log('I am done!');
}
  • $scope.chapter is used to define the current chapter.
  • $scope.current is used to define the current question from the current chapter user is seeing.

Is there a way of making this more elegant/simpler?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Before making it more elegant or simpler, I would strongly suspect that this logic isn't even correct. Please add more context to explain what you are trying to accomplish. How are $scope.current and $scope.chapter initialized? What is $scope.all? Done with what? Does this code run in a loop or an event handler? I've tried to revise the generic title to meet site requirements — see How to Ask. \$\endgroup\$ Commented Oct 20, 2015 at 17:48
  • \$\begingroup\$ What happens if you're not done? What's the context of this code? It would help alot to have this in order to give a better review! \$\endgroup\$
    – IEatBagels
    Commented Oct 20, 2015 at 20:11

3 Answers 3

1
\$\begingroup\$

I'd lean toward something like this:

if ($scope.current < $scope.questions.length - 1) {
  $scope.current++;
} else {
  if ($scope.chapter < $scope.all.length - 1) {
    $scope.current = 0;
    $scope.chapter++;
  } else {
    $scope.done = true;
    console.log('I am done!');
  }
}

Notes:

  • This assumes that $scope.current starts out being < $scope.questions.length

  • And that $scope.chapter starts out being < $scope.all.length

  • In general, barring a good reason otherwise, I recommend using increment operators rather than += 1. Either prefix or postfix is fine (or disregarding this advice entirely).

\$\endgroup\$
3
  • \$\begingroup\$ Nested "if/else" statements are not a good recommendation when the goal is readability. \$\endgroup\$
    – Jonah
    Commented Oct 20, 2015 at 19:32
  • \$\begingroup\$ Its there a reason why you recommend to use ++ instead of += ? \$\endgroup\$
    – Hiero
    Commented Oct 21, 2015 at 6:44
  • \$\begingroup\$ @Hiero: Same reason I would use it over $scope.current = $scope.current + 2 - 1; :-) Simplicity, conciseness, readability, and semantics -- all of which (except, perhaps, conciseness) are matters of opinion, and people can have good faith differences of opinion. To me, ++ says specifically that you're incrementing something. That said, the semantic difference between a++ and a += 1 is really small. (Side note: I prefer ++a [prefix], e.g. "increment a," over a++ [postfix], not entirely sure why I used postfix in the answer. There's no reason to in the above.) \$\endgroup\$ Commented Oct 21, 2015 at 7:12
3
\$\begingroup\$

The problem I encountered with this code is it doesn't say anything about what it does. All it tells me is it's adding values, subtracting values, flipping boolean values etc. It would be nice if you broke them down into functions with sensible names.

// Define logic
function isReaderDone(){/* logic here */}
function getUpdatedChapter(){/* logic here */}
function getUpdatedQuestionIndex(){/* logic here */}

// Update state
$scope.chapter  = getUpdatedChapter();
$scope.question = getUpdatedQuestionIndex();
$scope.done     = isReaderDone();

// Misc.
if($scope.done) console.log('I am done!');

The nice thing about the above is that it doesn't mash logic into each other. Unlike your code where current is updated in 3 separate places, there a clear distinction of when a variable is updated and what conditions change the value explicitly.

Another thing is that current doesn't really tell me what it is. Other devs be like "Current what? Current chapter? Current page? Current time?". your description defines it as a question, so name it that way.

I also notice you use ==. It's safer in JS to use strict comparison (===) instead of loose comparison. You don't want to end up in cases similar to '' == false (which is true by the way).

Elegant/simpler doesn't always equate to shorter code (that's why we have what we call a minifier for that). Elegance is writing a good balance of short, yet understandable code.

\$\endgroup\$
0
\$\begingroup\$

First, I would rename that $scope.current variable since $scope.chapter also refers to a current index - how about $scope.question instead? (I even debated going with $scope.question_index and $scope.chapter_index for clarity.) I also renamed $scope.done to $scope.is_done because I think the "is_" prefix makes my booleans instantly identifiable as booleans.

Next, I would improve readability by assigning those .length look-ups to friendly named variables, like num_questions and num_chapters. I don't like the inconsistency caused by the fact that the limit on questions is $scope.questions.length, but the limit on chapters is $scope.all.length - 1 - that "- 1" is awkward and it looks wrong to me, but I have left it in my answer in case it has to be there. I have also renamed $scope.all to $scope.chapters for clarity.

I think a lot of the redundancy in the conditionals can be replaced with else if and an else. The code in my answer assumes that $scope.question cannot be greater than num_questions and that $scope.chapter cannot be greater than num_chapters. If you wanted to take a more defensive position, you could throw an error if either was greater than its limit.

Finally, I cannot understand the purpose of decrementing $scope.current when we reach the "done" block, so I have omitted it.

var num_questions = $scope.questions.length;
var num_chapters = ($scope.chapters.length - 1);

if ($scope.question < num_questions) {
    $scope.question += 1;
}
else if ($scope.chapter < num_chapters) {
    $scope.question = 0;
    $scope.chapter += 1;
}
else {
    $scope.is_done = true;
    console.log('I am done!');
}
\$\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.