Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages in "triangle" #525

Closed
siebenschlaefer opened this issue May 2, 2022 · 2 comments · Fixed by #726
Closed

Improve error messages in "triangle" #525

siebenschlaefer opened this issue May 2, 2022 · 2 comments · Fixed by #726

Comments

@siebenschlaefer
Copy link
Contributor

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?

@KevDi
Copy link
Contributor

KevDi commented Jul 1, 2022

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?

vaeng added a commit that referenced this issue Oct 17, 2023
BREAKING CHANGE: forces students to use enums

closes #525
@vaeng
Copy link
Contributor

vaeng commented Oct 17, 2023

I think this is a very helpful change. I made a PR: #726

@vaeng vaeng closed this as completed in #726 Dec 3, 2023
vaeng added a commit that referenced this issue Dec 3, 2023
BREAKING CHANGE: forces students to use enums

closes #525
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants