Added comparison functions (GT/LT) to dice: !roll 1d20 + x > y #21
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
| } else { | ||
| text = ' Failure! ' + result + ' is higher than ' + target; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
could the quantifier be equal or not equal to? or what about GTE/LTE?
- Changed the checkTarget function to use different, cooler syntax. - Included E, GTE and LTE checks.
|
Updated pull request with suggestions. |
| }; | ||
| if (quantifiers[quantifier.toLowerCase()](parseFloat(result), parseFloat(target))) text = ' `Success!`'; | ||
| return text; | ||
| } |
There was a problem hiding this comment.
this is getting nitpicky, there really isnt anything wrong with what you have (except for maybe the parsefloat()) but here is how I would write this function:
function checkTarget(result, quantifier, target) {
//do your argument manipulation, defaulting, etc at the top of the function
//making your use of them more clear
quantifier = quantifier.toLowerCase();
//either use lodash's parseFloat or use parseFload(result, 10)
result = _.parseFloat(result);
target = _.parseFloat(target);
let quantifiers = {
//personal style preferences
'>': (a, b) => a > b,
'<': (a, b) => a < b,
'>=': (a, b) => a >= b,
'<=': (a, b) => a <= b,
'=': (a, b) => a === b
};
//this line is much easier to read with the manipulation stuff already done
if (quantifiers[quantifier](result, target)){
//I personally only use bracket-less if statements when doing an early bail-out return,
//like when the function should just not run - errors or something like that
return ' `Success!`';
}
//no need to store the result, just return it where necessary
return ' `Failure!`'
}There was a problem hiding this comment.
more info on parseInt() http://stackoverflow.com/questions/16880327/why-am-i-getting-weird-result-using-parseint-in-node-js-different-result-from
you can also use Number() in most cases instead of parseInt
|
If/when this lands it will require updates to the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
critique welcome, as always.
I think checkTarget() can be streamlined, but I'm not sure how