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

feat: add dnd-character exercise #802

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Conversation

vaeng
Copy link
Contributor

@vaeng vaeng commented Feb 12, 2024

No description provided.

@vaeng vaeng mentioned this pull request Feb 12, 2024
11 tasks
@vaeng vaeng added x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation labels Feb 12, 2024
@vaeng vaeng requested a review from ErikSchierboom February 14, 2024 12:29
@vaeng vaeng merged commit bd28108 into main Feb 15, 2024
9 checks passed
@vaeng vaeng deleted the feat--add-dnd-character-exercise branch February 15, 2024 16:29
@siebenschlaefer
Copy link
Contributor

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
Copy link
Contributor Author

vaeng commented Feb 23, 2024

Are you open to some late changes?

Sure, I'll do an extra PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants