6
\$\begingroup\$

I am not sure if I am just having a brain lapse here or if there is a better way to do this:

I am trying to only add one to count in the following scenarios:

  • the bool excludezeros is set to false (count should be increased for iterations of the foreach)
  • the bool excludezeros is set to true and studentScore is not equal to 0 (count only increases when student score is not equal to 0)

Here is what I have:

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;
    if (excludeZeroes && studentScore == 0)
    { 
        continue; 
    }
    else 
    { 
        count++; 
    }
}
\$\endgroup\$
  • \$\begingroup\$ Seems like a reasonable solution \$\endgroup\$ – rickythefox May 24 '16 at 12:11
  • \$\begingroup\$ the biggest problem I have with your solution is that you are summing the score even if you are not counting it toward the count. (I realize that the sum does not change as the score is 0 in this case, but what if later you extend that to, say, all scores below 5? or if some scores can become negative for some reason?) \$\endgroup\$ – njzk2 May 24 '16 at 19:16
19
\$\begingroup\$

I agree with forsvarir's answer, that you should lose the continue by inverting the condition. But you can do one better than that: lose the loop!

var scores = students.Select(student => GetScore(student));
int count = scores.Count(score => !excludeZeros || score != 0);
int total = scores.Sum();

Done. No loop needed.

Or equivalently:

var scores = from student in students
             let score = GetScore(student)
             where !excludeZeros || score != 0
             select score;
int count = scores.Count();
int total = scores.Sum();

You know what is really nice about doing it this way? First, the code looks like what it means; it's closer to the "business domain" of the code. Second, it is super easy to extract more statistics:

int maximum = scores.Max();
int average = scores.Average();
var sorted = scores.OrderBy(x=>x);
var bottomTen = sorted.Take(10);

and so on.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I disagree with the first part of your answer, because you create a strong dependency between the score value (0), and the value of the scores that are not counted (which happens to be 0 as well). I would replace the constraint in the count by a filter earlier in the process. \$\endgroup\$ – njzk2 May 24 '16 at 19:14
  • 2
    \$\begingroup\$ @njzk2: You make a good point! The two queries I construct here are NOT equivalent, because the counts and therefore the averages of both are different. \$\endgroup\$ – Eric Lippert May 24 '16 at 20:08
  • \$\begingroup\$ This will loop twice. \$\endgroup\$ – Stephane Mathis May 25 '16 at 9:04
  • 1
    \$\begingroup\$ @EricLippert I've seen you on code review more and more. Please don't stop! \$\endgroup\$ – Insane May 25 '16 at 9:59
  • \$\begingroup\$ A+ answer, Eric! \$\endgroup\$ – rookie May 25 '16 at 12:51
5
\$\begingroup\$

With the current approach, you don't need the else, the loop skips to the next iteration on continue:

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;
    if (excludeZeroes && studentScore == 0) {
        continue;
    }

    count++;
}

Or you could negate the condition and lose the continue altogether:

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;
    if (studentScore != 0 || !excludeZeroes) {
        count++;
    }
}
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

You can accomplish this either by doing

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;

    if (!excludeZeroes || studentScore != 0)
    {
        count++;
    }
}

or (perhaps a more clear way)

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;

    if (!excludeZeroes ||
       (excludeZeroes && studentScore != 0))
    {
        count++;
    }
}

In the first choice, we put the !excludeZeroes check first to short-circuit the check. That way, the second condition, studentScore != 0 only gets evaluated if the first condition !excludeZeroes returns false (or rather, if excludeZeroes is true).

In the second choice, we explicitly define what conditions we want it to occur, and nothing is implied.

You might also consider avoiding using double-negatives, and replace excludeZeroes with includeZeroes, so that the code looks like

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;

    if (includeZeroes || studentScore != 0)
    {
        count++;
    }
}

or

foreach (var student in students)
{
    double studentScore = GetScore();
    score += studentScore;

    if (includeZeroes ||
       (!includeZeroes && studentScore != 0))
    {
        count++;
    }
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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