This repository was archived by the owner on Oct 23, 2019. It is now read-only.
Refactored gaming plugin for project consistency and rewrote coin flip function #391
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A-UNDERSCORE-D
previously approved these changes
Feb 5, 2019
linuxdaemon
suggested changes
Feb 5, 2019
linuxdaemon
suggested changes
Feb 5, 2019
leonthemisfit
changed the title
linuxdaemon
suggested changes
Mar 18, 2019
leonthemisfit
force-pushed
the
gonzobot+gaming
branch
from
245a814 to
bd7aaf8
Compare
Updates the docstrings in the gaming plugin to be more consistent with the rest of the project.
The user facing strings were hardcoded into the function rather than being constants. Having them as constants improves readability and maintainability.
The original way of calculating coin flips was more complex and harder to read than needed. This changes it to use a simpler and cleaner method that's still statistically close enough to actual coin flip results.
The clamp function was originally necessary to deal with some issues with the coin flip function. The new logic for coin flip makes this no longer necessary and therefore it can be removed.
Added my name (leonthemisfit) to the modified by comment because of making significant changes.
There was a typo in the new coin flip logic where it was supposed to run a proper simulation if flips were under the roll limit but instead would only run if they were over. Also moved hardcoded roll limit to a constant because it is used in multiple places.
As per request the format strings utilizing a single quote
within the string have been modified to remove the quotes
and use the {!r} formatter. The string for invalid rolls has
also been moved to a constant for consistency with the
rest of the plugin as well as improving readability.
The n_rolls function was incorrectly checking if the number of sides was less than the roll limit rather than the number of rolls to perform.
This adds the ability to approximate fudge rolls greater than the roll limit in n_rolls. This also majorly refactors n_rolls because the changes to fudge dice make it possible to use the approximation calculations in a more generic way that is applicable to both fudge and non fudge dice.
The wrong variable was being checked resulting in simulating very large numbers rather than approximating them. This patch fixes that.
leonthemisfit
force-pushed
the
gonzobot+gaming
branch
from
bd7aaf8 to
7d40650
Compare
This comment has been minimized.
This comment has been minimized.
Because 'F' is a letter and not a number if a person were to roll a fudge die less than the roll limit the call to random.randint would fail. This has been replaced with a correct checking of fudge dice and ensuring the correct values are passed. "Uh, Big Bird, F is not a number... F is a letter" --Kramit the Frog, Sensimilla Street
The current location of the explanatory comment is actually beneath a large chunk of where the approximation happens. This is remedied by moving the comment above those approximations.
The original parameter names were unclear and has already caused confusion over the process of cleaning this up, so a quick refactor has been done to clear up confusion.
leonthemisfit
changed the title
linuxdaemon
suggested changes
Mar 18, 2019
linuxdaemon
suggested changes
Mar 19, 2019
The two places that int() was used to get the floor of a number has been replaced with round because linuxdaemon.
This simply adds a return type in the docstring of the nrolls function in gaming.py.
linuxdaemon
suggested changes
Mar 20, 2019
There was a problem hiding this comment.
Please add some unit tests, something along the lines of https://gist.github.com/linuxdaemon/29cad9c3371f1e9182f3686d63a72170 should work.
In order to provide better documentation and readability as to how rolls are approximated the formulas have been moved to their own functions. This allows each formula to be better documented in docstrings as well as giving them concise names so that there's no reliance on just variable names to understand how the values are being found.
In order to continue to improve readability and testability the approximation of rolls has been moved to a separate function. This allows n_rolls to be shorter and more readable.
The last step in this refactor is to move dice roll simulation to a separate function. This final piece of refactoring represents all logic being separated into logical units. This provides the best means to document and test the various functions. This also provides better maintainability.
Rather than having multiple return statements which can often cause confusion, the results of the two function calls are stored and then returned at the end of the code block. This can also help future changes where the returned values may be manipulated for additional features or other unforeseen reasons.
4 digits is plenty of precision for the calculations that use this to be accurate. This is also done so that tests in docstrings will be easier to read.
Like find_variance 4 digits of precision is plenty for its use here and it improves readability of test results.
This commit adds simple tests/examples to the docstrings of util functions that are non-random (for simplicity) so that you can get an idea of what kind of data they return.
There was a discrepancy between my local test results and the test results from travis with the return type from find_midpoint. The tests in the plugin have been updated to reflect the results travis expects rather than my local machine.
linuxdaemon
approved these changes
Mar 20, 2019
linuxdaemon
suggested changes
Mar 20, 2019
There was a problem hiding this comment.
Please expand test coverage according to https://codecov.io/gh/snoonetIRC/CloudBot/pull/391/src/plugins/gaming.py
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The plugin's docstrings were not consistent with the rest of the project and have
been updated to better reflect that. The coin flip function has also been rewritten
with new logic that estimates large flip amounts in a cleaner more maintainable
manner while still providing very similar results to the original method.