cpp icon indicating copy to clipboard operation
cpp copied to clipboard

Improve error messages in "triangle"

Open siebenschlaefer opened this issue 2 years ago • 1 comments

The tests in the exercise "triangle" compare the result of the function with the operator== like this:

TEST_CASE("equilateral_triangles_have_equal_sides")
{
    REQUIRE(triangle::flavor::equilateral == triangle::kind(2, 2, 2));
}

By default Catch2 prints an enum or enum class like an integer:

-------------------------------------------------------------------------------
equilateral_triangles_have_equal_sides
-------------------------------------------------------------------------------
/home/user/exercism/cpp/triangle/triangle_test.cpp:9
...............................................................................

/home/user/exercism/cpp/triangle/triangle_test.cpp:11: FAILED:
  REQUIRE( triangle::flavor::equilateral == triangle::kind(2, 2, 2) )
with expansion:
  0 == 1

That 0 == 1 is neither expressive nor helpful.

Also, I've mentored many solutions that used something other than an enum, e.g.:

    namespace flavor {
        const int equilateral = 0;
        const int isosceles = 1;
        const int scalene = 2;
    }
    int kind(double side1, double side2, double side3);

// or

    class flavor {
        public:
        static const std::string isosceles;
        static const std::string equilateral;
        static const std::string scalene;
    };
    std::string kind(double a, double b, double c);

Catch2 has the macro CATCH_REGISTER_ENUM() for better error messages when working with enums (see the documentation).

By adding a few lines somewhere at the beginning of triangle_test.cpp:

// for better error messages
CATCH_REGISTER_ENUM(triangle::flavor,
        triangle::flavor::equilateral,
        triangle::flavor::isosceles,
        triangle::flavor::scalene)

we would get better error messages:

-------------------------------------------------------------------------------
equilateral_triangles_have_equal_sides
-------------------------------------------------------------------------------
/home/user/exercism/cpp/triangle/triangle_test.cpp:15
...............................................................................

/home/user/exercism/cpp/triangle/triangle_test.cpp:17: FAILED:
  REQUIRE( triangle::flavor::equilateral == triangle::kind(2, 2, 2) )
with expansion:
  equilateral == isosceles

and that would guide students towards using enum or enum class.


AFAIK there are only three possible problems:

  1. When the students read the tests they will be guided towards an enum or enum class, they are less likely to "discover" enums on their own when searching for the best solution. But I'd rather view that "guidance" as positive.

  2. That would effectively force the use of enum or enum class, any of those alternative approaches (see above) would lead to a compilation error. IMHO that's not a problem for us because we want idiomatic solutions and that's enum or better enum class.

  3. If the students want to add a fourth degenerate enumerator (for the bonus part in "Dig Deeper"), they will have to add that enumerator in the tests, too, or they wil get this "unexpected enum value":

    -------------------------------------------------------------------------------
    equilateral_triangles_have_equal_sides
    -------------------------------------------------------------------------------
    /home/user/exercism/cpp/triangle/triangle_test.cpp:15
    ...............................................................................
    
    /home/user/exercism/cpp/triangle/triangle_test.cpp:17: FAILED:
      REQUIRE( triangle::flavor::equilateral == triangle::kind(2, 2, 2) )
    with expansion:
      equilateral
      ==
      {** unexpected enum value **}
    

IMHO the benefits outweigh these problems. What do you folks think?

siebenschlaefer avatar May 02 '22 19:05 siebenschlaefer

I think the point with the enum/enum class is a valid option. Especially if the solution already tend to point to an enum/enum class. So why not push the users in the right direction with that?

KevDi avatar Jul 01 '22 19:07 KevDi

I think this is a very helpful change. I made a PR: https://github.com/exercism/cpp/pull/726

vaeng avatar Oct 17 '23 11:10 vaeng