sum-of-multiples: Add up to 7 test case

[jpreese]

I'd like to propose adding an additional test case to this exercise (as well as simplify the earlier ones). I believe these test cases would be a more logical approach when tackling this problem using TDD.

Currently, for the 3rd step, it takes a bit of a leap. It's the first time we actually have to worry about the 5 (since in previous cases it could never be possible) as well as having to sum 3 multiple times to return a result of 23.

This new test case is a little more gentle in the sense that we don't need to worry about another factor, we just need to worry about 3 + 6.

After that, we can introduce 5 (and other factors) because the student should have accomplished summing 3 multiple times, allowing them to move onto a test case that leverages multiple factors.

[Insti]

I like the changes you're proposing here, adding the single number tests first.

But I think this test is still relevant, with one number below the limit and one number above.

[jpreese]

I personally just don't think it brings much value when the solution to the test is just going to be

if(limit > 3} return 3;

Until you knock out the next couple of tests which force you to refactor, and replace 3 with facotrs[].

I'm not necessarily sold on it, but just my opinion.

[Insti]

Maybe not the way that you are implementing the solution, but there are many different approaches. In some of these this will be an issue.

[jpreese]

I've put the 5 back for all of the cases except the first. It doesn't quite solve what I was trying to do, but I think it's just going to boil down to opinion.

[Insti]

What is this testing that is different from the 3 or 5 up to 10 test?

It is good if the test describes what it is testing rather than repeating the input information.

[jpreese]

It was in my previous commit, but you had mentioned you liked the 2nd test case with the 5 as shown in the original, and I don't think it's a good idea to back and forth between inputting 1 factor and 2 factors. This test is supposed to be focusing on just summing the single factor.

Let me give a little overview how I see this problem getting solved, using the TPP.

Test #1 will simply be returning 0 (nil -> constant)

    public static int Sum(int[] multiples, int max)
    {
        return 0;
    }

Test #2 will be splitting the execution path, adding the if statement for an input of 3 (statement -> conditional)

    public static int Sum(IEnumerable<int> multiples, int max)
    {
        if(max > 3)
        {
            return 3;
        }

        return 0;
    }

Test #3 will be needing to Sum 3 and 6 to make 9. (statement ->conditional)

public static int Sum(IEnumerable<int> multiples, int max)
{
	if(max > 6)
	{
		return 9;
	}

	if(max > 3)
	{
		return 3;
	}

	return 0;
}

At this point, we have duplication. We have 3s and 6s, we're trying to add to the sum multiple times, we use if -> loop

    public static int Sum(IEnumerable<int> multiples, int max)
    {
        int sum = 0;

        for (int x = 1; x < max; x++)
        {
            if (x % 3 == 0)
            {
                sum += x;
            }
        }

        return sum;
    }

At this point, we have solving for a factor of 3 implemented. The loop is there. The modulus is there. We just need to worry about introducing another factor and nothing more.

That's kind of where I'm trying to get with the introduction of a new case. I want to focus on the case where the only factor is 3 and the limit is 7. This case forces the student to sum 3 + 6 and not worry about an array of factors until Test #4. Before that, I like the idea of only having 3 as a factor.

[Insti]

I have no problem with single factor cases. (I think they are a good idea.)
But they can be added to the start of the test cases without removing/changing any of the existing cases.

[Insti]

(This was a pre-existing issue, so not your fault at all, but the test descriptions for this problem are not helpful.)

[Insti]

Your TDD examples are helpful, but you are relying on the factor being 3 the whole time. Using a different approach I would build based on changing factors before changing limits, which leads to different tests being useful at different times.

What would you do if the the second and third tests were:
"factors": [3], "limit": 7,
"factors": [5], "limit": 7,

[jpreese]

Limit 4 would make more sense for the 2nd test, so we'll assume typo :)

But this was my initial proposal. So my tests are above. In order to make test 4 pass I would just change 3 to be more generic, multiples[0] (constant -> scalar)

[Insti]

Right, so ADD these tests to the start of the json file.
But don't change/remove the existing tests, then everything should be fine.

[jpreese]

I feel like if we just added those four cases we'd just be bloating the test suite. If you pass those tests, the logic that we're verifying will have already been implemented. That's why I initially just removed 5 from the factors array and then introduced it in a later case.

[Insti]

So what do you want to do with this PR?

I'm OK with a PR that only adds tests.
I'm OK with discussing a PR that removes tests.
I seem to be against a PR that does both.

What about turning this one into one that only adds new "simpler" tests, and then creating a new PR to remove the tests you then think are redundant?

[jpreese]

I'm not in a rush and I welcome the critique/discussions! I hopefully recruited a couple more eyes on this so we can get some more opinions and figure out what is the correct approach.

So lets just leave it for now, and we'll go from there if you're OK with that 👍

[Insti]

Why do you want to change this test?
Why not add the new "simpler" test cases that you proposed?

[Vankog]

(Anything wrong if we continue the discussion here in the main thread? It's easier to follow.

I'm OK with a PR that only adds tests.
I'm OK with discussing a PR that removes tests.
I seem to be against a PR that does both.

Haha, nice to see that I'm not the only one with this issue in my PRs ^^
Although, I'm not quite sure why this is an issue to you, @Insti.
This particular request contains a relatively simple addition and deletion. So it's not that hard to review. It's not a complicated matter so a separation into two PRs seems like a bit of an overkill to me in this particular case.
BTW: I prefer the split view diff over the unified view. It makes things easier to grasp.)

---

Back to topic:
I agree to have a single factor progression first, before stepping it up to multiple factors.
And I also see the benefit of having a sub-progression from zero summands to several summands. So @jpreese's suggestion seems totally fine to me.

So, I am pro-adding.

Now the deletion issue:
I don't see a reason to delete the original first two cases. To me these are the logical progression after your additions.
However, because the original cases already use 3 and 5, why don't you use another number for your single factor cases? Like 2 or 4 maybe? Or even both, one after another? This way the original cases don't seem so redundant any more.

So instead of your suggested

3 up to 1
3 up to 4
3 up to 7
3,5 up to 10
...

It could be

2 up to 1
4 up to 5
4 up to 9
3,5 up to 1
3,5 up to 4
3,5 up to 10
...

This will have the benefit of forcing the refactoring earlier, because 2 and 4 and later 3 and 5 are less likely to be hard-coded than just using 3 all the time.

[Insti]

This particular request contains a relatively simple addition and deletion. So it's not that hard to review. It's not a complicated matter so a separation into two PRs seems like a bit of an overkill to me in this particular case.

I agree with the addition and strongly disagree with the deletion.
So instead of having 1 PR easily merged and 1 focused on a single issue we get stuck in discussions for weeks with multiple threads about all the different changes and nothing gets improved.

[jpreese]

@Vankog @Insti I've just gone ahead and added the test for this PR. As it's been suggested previously, we can discuss the earlier tests in another PR.

[Insti]

Great 👍

Is this the best place for this test?
Is it simpler than the earlier cases?
Only having 1 factor makes it look simpler to me, but I'm not sure.

[jpreese]

I personally think it looks a little weird going back and forth between multiples of 3 and 5, and then a single 3 -- but following the approach I gave earlier, it makes the most sense. Again, to solve Test 1 and Test 2 all you're doing is:

    public static int Sum(IEnumerable<int> multiples, int max)
    {
        if(max > 3)
        {
            return 3;
        }

        return 0;
    }

That's a big reason to adjust the previous tests, mostly because I think you just completely ignore the 5 case anyway.

[ErikSchierboom]

@Insti What do you feel about @jpreese's explanation? Is that okay? If so, is this ready to be merged or are there still changes to me made?

[Insti]

Yes, this is fine.
The PR as it is currently is OK to merge.

[Vankog]

@jpreese

I've just gone ahead and added the test for this PR. As it's been suggested previously, we can discuss the earlier tests in another PR.

Hm... But you added just one case now. What was wrong with the others?

I, too, think that you should recheck if your suggested order is correct. It seems the single factor case might as well be in front of the two factor tests. (like in your previous submission or in my example from my previous post).

[jpreese]

@Vankog There should only be one test to add. I'm not sure which others you were referring to.

I think the order is OK. If you expand some of the earlier discussions, you'll actually see the approach I took to this problem using TDD.

[Vankog]

I took the code threads into account.
That's why I was asking about the order as well. It seemed to me that one-factor cases are easier to implement than multi-factor cases.

I am referring to your changed cases. I think they can be added too (instead of replacing).

[jpreese]

The changed tests were to facilitate the natural progression to the test that I would like to add, ignoring the 5 case until we were ready to implement more than one factor. I'd think adding 3 cases, instead of the one, would just be redundant.

[ErikSchierboom]

Thanks @jpreese !