Triangle: add smaller size equality test

Exercism Bugs & Feature Requests
1

Original PR

If the two smaller sides are equal, the code below will falsely classify the triangle as scalene rather than isosceles. The current tests do not test this case. This behavior is related to the sorting of the sides.

flavor kind(float a, float b, float c) {
    std::array<float, 3> sides{a, b, c};
    std::sort(sides.begin(), sides.end()); // Sides sorting
    if (!is_positive(sides)) {
        throw std::domain_error("Sides must be positive");
    }
    if (!triangle_inequality(sides)) {
        throw std::domain_error("The triangle inequality is violated");
    }
    if (sides[0] == sides[2]) {
        return flavor::equilateral;
    } else if (sides[1] == sides[2]) { // This check is incorrect
        return flavor::isosceles;
    } else {
        return flavor::scalene;
    }
}

This tests the case where the sides are sorted.

TEST_CASE("isosceles triangle -> two smaller sides are equal",
          "[eca8ae18 - e191 - 4795 - 836f - c6457e649d80]") {
    REQUIRE(triangle::flavor::isosceles == triangle::kind(3, 4, 3));
}
2

This PR would be better in the problem-specifications repo, so all tracks can benefit.

3

New PR in problem-specifications

4

I asked them to post here on the forum where all changes to the problem statement and the tests should be discussed before any changes are made.

5
  1. The tests will never catch every bug. How prevalent is this approach and is it worth adding a test?
  2. If we want to be more comprehensive and cover these additional scenarios, we might want to be comprehensive about it and add all orderings: [(3, 3, 4), (3, 4, 3), (4, 3, 3)].
1 Like
6
  1. I don’t know how prevalent this solution is. I found it in JS and C++. I just didn’t look for it in others.
  2. You are right, I will add this.
7

If it’s not prevalent, the maintainers may deem this isn’t worth adding ;) Please wait for some sort of consensus before opening/modifying any PRs.

8

This part of the docs may be relevant here.

1 Like
9

I’m not convinced. This edge case occurs when the sides are sorted and you use the sorted sides for determining triangle type by only doing one comparison for isosceles. So this is an implementation detail that can be addressed in a mentoring discussion.

In my experience mentoring triangle (32 times across 9 tracks), I don’t recall a student using sorted sides in this fashion for isosceles triangles. I do see students sort the sides to optimize triangle inequality checks, but then they check whether any two sides or equal to each other. That setup avoids this issue.

1 Like
10

I’ve mentored a few solutions that used the sorted sides to determine the “kind” of the triangle. But I do not remember a solution with this issue where only one pair is checked for equality.

11

Same.

I do like to sort the sides. It makes the inequality test a lot simpler.

3 Likes
12

Checked the canonical-data and the C++ implementation. It appears that there’s a test case in the canonical-data that’s not included in the C++ track. This one

        {
          "uuid": "3da23a91-a166-419a-9abf-baf4868fd985",
          "description": "first and third sides are equal",
          "property": "scalene",
          "input": {
            "sides": [3, 4, 3]
          },
          "expected": false
        },

Last time that the tests were modified in the C++ track was 4 years ago. So it looks like the tests are out of sync. With the addition of this test, the code posted by the OP will classify the triangle as scalene and fail.

1 Like
13

Triangle was synced last October per update triangle tests by DebbieSan · Pull Request #787 · exercism/cpp · GitHub, and that first and third sides are equal test was excluded. So that’d be up to the maintainer if they want to revisit that.

1 Like
14

Correct, my bad. I was looking at the tests folder that indicated a sync 4 years ago.

15

Except that [3, 4, 3] test is a negative test for scalene. This proposal is to make it a positive test for isosceles.

I’m in favour.

16

I’m ambivalent.

Is this an important enough strategy that we think a sizable amount of students (across tracks) would choose it and therefore need “reminding” through additional tests?

I (personally) don’t think it’s going to invalidate many solutions, but I will defer to those who’ve mentored the exercise more than I have.

Re-testing 18,803 solutions on the Python track might take a bit – but I probably wouldn’t update the exercise from canonical data right away anyway. :woman_shrugging:

17

Oh yes, great point. But now there’s an issue with the specs. The description states:

“Determine if a triangle is equilateral, isosceles, or scalene.”

That suggests that it suffices to check if triangle is classified correctly. However, some tests check what a triangle isn’t rather than what it is.

For example, there’s a test for [5, 4, 6] under equilateral, which returns false. This confirms that the triangle isn’t equilateral, but it doesn’t tell us what type it actually is.

If the goal is to correctly classify triangles, then the tests should be consistent — all of them should check for true return values. But many tests don’t seem to follow this approach. For instance, [1, 1, 3] has the property isosceles and expects false as the return value.

So, what’s the actual intent of this exercise? Should implementations classify triangles correctly? Or are they simply answering the question, “Is this triangle of type X?”

18

They should probably do what they’re already doing ;) I don’t think it makes sense for existing exercises with any significant number of solutions to change what they’re doing.

3 Likes
19

The latter. An equilateral triangle is also an isosceles triangle according to the instructions. If a track wants a classification returned, not all the tests can be included because of that ambiguity.

1 Like