exercism / cpp

Exercism exercises in C++.
https://exercism.org/tracks/cpp
MIT License
251 stars 205 forks source link

feat: add dnd-character exercise #802

Closed vaeng closed 4 months ago

siebenschlaefer commented 4 months ago

Are you open to some late changes?


REQUIRE((character.strength >= 3 && character.strength <= 18)); does result in test results that just say that the test has failed:

-------------------------------------------------------------------------------
random character is valid
-------------------------------------------------------------------------------
/vagrant/exercism/cpp/dnd-character/dnd_character_test.cpp:80
...............................................................................

/vagrant/exercism/cpp/dnd-character/dnd_character_test.cpp:82: FAILED:
  REQUIRE( (character.strength >= 3 && character.strength <= 18) )
with expansion:
  false

Instead you could create a "Matcher":

template <typename T>
class IsBetweenMatcher : public Catch::MatcherBase<T> {
    T m_begin, m_end;
public:
    IsBetweenMatcher(T begin, T end) :
        m_begin(begin), m_end(end) {
    }
    bool match(T const& in) const override {
        return in >= m_begin && in <= m_end;
    }
    std::string describe() const override {
        std::ostringstream ss;
        ss << "is between " << m_begin << " and " << m_end;
        return ss.str();
    }
};

and we would get error messages like this:

-------------------------------------------------------------------------------
random character is valid
-------------------------------------------------------------------------------
/vagrant/exercism/cpp/dnd-character/dnd_character_test.cpp:103
...............................................................................

/vagrant/exercism/cpp/dnd-character/dnd_character_test.cpp:105: FAILED:
  CHECK_THAT( character.strength, IsBetweenMatcher(3, 18) )
with expansion:
  31 is between 3 and 18

/vagrant/exercism/cpp/dnd-character/dnd_character_test.cpp:106: FAILED:
  REQUIRE( (character.strength >= 3 && character.strength <= 18) )
with expansion:
  false

What is the purpose of the last test ("each ability is only calculated once")?

That test makes sense in languages like Python or C# where some property look like a member variable but actually some getter function gets invoked. But here in C++ can a test like REQUIRE(character.strength == character.strength); fail at all?

I'd rather suggest removing that test.


There is a comment

//REQUIRE("score >= 3 && score <= 18" == dnd_character::ability());

that looks like an artifact from the development process. Do we need it?


I think the implementation of ability() in the example solution is incorrect.
It throws three dice and returns their sum. But actually it should throw four dice, discard the smallest, and return the sum of the other three dice.

vaeng commented 4 months ago

Are you open to some late changes?

Sure, I'll do an extra PR.