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

Fixed marked_cards_count #57

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MimmyJau
Copy link
Contributor

I believe this fixes the issue of marked_cards_count() not working if no cards are marked.

I am willing to build out some tests to make sure this works as expected, however I may need some guidance on this as there are currently no tests for keyboard_test.c.

Open to any feedback. Thanks.

@mpereira
Copy link
Owner

Hi @MimmyJau,

Thanks for working on this!

Do you know if the issue referenced in the FIXME comment manifests itself somehow in the game? I can't remember.

Do you wanna have a crack at writing a simple test for this? Maybe card_test.c could be a good reference? Let me know if there's something specific I can do to help out.

@MimmyJau
Copy link
Contributor Author

Hi @mpereira, the game still works fine without this change! Or at least I never came across any issues.

My reason for changing it came from thinking about a potential re-implementation of handle_card_movement() as discussed in PR #53, and one idea would involve a function that distinguishes when cards are marked from when they're not.

And sounds good! Happy to take a first pass at writing a test. Would it be best to have this test in keyboard_test.c or some other file? Will let you know if I have any other questions.

@MimmyJau
Copy link
Contributor Author

This is a first attempt at writing tests for marked_cards_count() inkeyboard_test.c. The three tests are:

  1. Returns 0 on an empty stack,
  2. Returns 0 on a non-empty stack with no marked cards [old implementation should fail where new implementation succeeds], and
  3. Returns a number from 1-13 depending on how many cards are marked.

Notes / Questions:

  • I had to remove the static keyword for the marked_card_count() definition in order to get access to it in keyboard_test.c. I'm not sure if this creates other issues elsewhere or if this is the best solution.
  • For the 2nd and 3rd tests, I initialized two stacks, with all the cards starting in the first stack and using move_card() to the second stack. I couldn't come up with a better way for getting each card to have their begin_y be + 1 compared to the previous card, but if there is let me know.
  • Are there other tests I should run?

@mpereira
Copy link
Owner

Hi @MimmyJau,

Thanks for the tests! I'll make some time to look into your changes this weekend.

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 this pull request may close these issues.

2 participants