projectmoon
/
tenebrous-dicebot
1
0
Fork 1
Code Issues 12 Pull Requests 1 Projects Releases 10 Wiki Activity

Keep/Drop Function #92

Manually merged
kg333 merged 12 commits from kg333/tenebrous-dicebot:keep_drop_function into master 2021-09-26 14:05:57 +00:00
Conversation 2 Commits 12 Files Changed 4 +20 -15
2 changed files with 20 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 2654887d8c - Show all commits

9
dicebot/src/basic/dice.rs
View File

@ -12,17 +12,22 @@ use std::ops::{Deref, DerefMut};
pub struct Dice {
pub(crate) count: u32,
pub(crate) sides: u32,
pub(crate) keep: u32,
}
impl fmt::Display for Dice {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if self.keep == self. count {
write!(f, "{}d{}", self.count, self.sides)
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-22 19:51:54 +00:00
Outdated
Review
Copy Link

Since you have if-else clauses here, it might make more sense to have a third enum member in addition to Keep and Drop, which indicates that we do nothing special with the roll.

Since you have if-else clauses here, it might make more sense to have a third enum member in addition to Keep and Drop, which indicates that we do nothing special with the roll.
kg333 commented 2021-09-25 03:04:43 +00:00
Outdated
Review
Copy Link

Added another enum member called None and it's much cleaner. Although members with no type give my C-programmer soul the heebie-jeebies.

Added another enum member called None and it's much cleaner. Although members with no type give my C-programmer soul the heebie-jeebies.
} else {
write!(f, "{}d{}k{}", self.count, self.sides, self.keep)
}
}
}
impl Dice {
pub fn new(count: u32, sides: u32) -> Dice {
Dice { count, sides }
pub fn new(count: u32, sides: u32, keep: u32) -> Dice {
Dice { count, sides, keep }
}
}

24
dicebot/src/basic/parser.rs
View File

@ -36,7 +36,7 @@ fn parse_dice(input: &str) -> IResult<&str, Dice> {
let (input, (count, _, sides)) = tuple((digit1, tag("d"), digit1))(input)?;
Ok((
input,
Dice::new(count.parse().unwrap(), sides.parse().unwrap()),
Dice::new(count.parse().unwrap(), sides.parse().unwrap(), count.parse().unwrap()),
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-17 06:51:15 +00:00
Outdated
Review
Copy Link

I think it is better to document what the keep expression actually is here, so tacking on "1dXkY" would be useful.

And since pattern matches are expressions, I think we can rewrite these two match blocks. Something like this:

let (keep, input) = match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) {
    // if ok, keep expression is present
    Ok(r) => (r.0, r.1.1),
    // otherwise absent and keep all dice
    Err(_) => ("".to_string(), count)
};

The same should be doable for the drop expression. I think this way you can also get rid of the mut on input, because let in rust is not just a variable assignment. It actually creates a whole new variable binding.

Note: I haven't tested the above, but it should work. Or some variation of it should work.

I think it is better to document what the keep expression actually is here, so tacking on "1dXkY" would be useful. And since pattern matches are expressions, I think we can rewrite these two match blocks. Something like this: ```rust let (keep, input) = match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) { // if ok, keep expression is present Ok(r) => (r.0, r.1.1), // otherwise absent and keep all dice Err(_) => ("".to_string(), count) }; ``` The same should be doable for the drop expression. I think this way you can also get rid of the `mut` on input, because `let` in rust is not just a variable assignment. It actually creates a whole new variable binding. Note: I haven't tested the above, but it should work. Or some variation of it should work.
))
}
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-22 19:53:33 +00:00
Outdated
Review
Copy Link

Rust supports tuple destructuring in pattern matching, which may be useful here and the other match:

Ok((rest, (_, keep_amount)) => ...

rest being the rest of the input in this case.

Rust supports tuple destructuring in pattern matching, which may be useful here and the other match: ```rust Ok((rest, (_, keep_amount)) => ... ``` `rest` being the rest of the input in this case.
kg333 commented 2021-09-25 02:58:04 +00:00
Outdated
Review
Copy Link

Ooh, that's much more human-readable, thanks!

Ooh, that's much more human-readable, thanks!
@ -108,16 +108,16 @@ mod tests {
use super::*;
#[test]
fn dice_test() {
assert_eq!(parse_dice("2d4"), Ok(("", Dice::new(2, 4))));
assert_eq!(parse_dice("20d40"), Ok(("", Dice::new(20, 40))));
assert_eq!(parse_dice("8d7"), Ok(("", Dice::new(8, 7))));
assert_eq!(parse_dice("2d4"), Ok(("", Dice::new(2, 4, 2))));
assert_eq!(parse_dice("20d40"), Ok(("", Dice::new(20, 40, 20))));
assert_eq!(parse_dice("8d7"), Ok(("", Dice::new(8, 7, 8))));
}
#[test]
fn element_test() {
assert_eq!(
parse_element(" \t\n\r\n 8d7 \n"),
Ok((" \n", Element::Dice(Dice::new(8, 7))))
Ok((" \n", Element::Dice(Dice::new(8, 7, 8))))
);
assert_eq!(
parse_element(" \t\n\r\n 8 \n"),
@ -139,14 +139,14 @@ mod tests {
parse_signed_element(" \t\n\r\n- 8d4 \n"),
Ok((
" \n",
SignedElement::Negative(Element::Dice(Dice::new(8, 4)))
SignedElement::Negative(Element::Dice(Dice::new(8, 4, 8)))
))
);
assert_eq!(
parse_signed_element(" \t\n\r\n+ 8d4 \n"),
Ok((
" \n",
SignedElement::Positive(Element::Dice(Dice::new(8, 4)))
SignedElement::Positive(Element::Dice(Dice::new(8, 4, 8)))
))
);
}
@ -158,7 +158,7 @@ mod tests {
Ok((
"",
ElementExpression(vec![SignedElement::Positive(Element::Dice(Dice::new(
8, 4
8, 4, 8
)))])
))
);
@ -167,7 +167,7 @@ mod tests {
Ok((
" \n ",
ElementExpression(vec![SignedElement::Negative(Element::Dice(Dice::new(
8, 4
8, 4, 8
)))])
))
);
@ -176,11 +176,11 @@ mod tests {
Ok((
" 1d5 ",
ElementExpression(vec![
SignedElement::Positive(Element::Dice(Dice::new(3, 4))),
SignedElement::Positive(Element::Dice(Dice::new(3, 4, 3))),
SignedElement::Positive(Element::Bonus(7)),
SignedElement::Negative(Element::Bonus(5)),
SignedElement::Negative(Element::Dice(Dice::new(6, 12))),
SignedElement::Positive(Element::Dice(Dice::new(1, 1))),
SignedElement::Negative(Element::Dice(Dice::new(6, 12, 6))),
SignedElement::Positive(Element::Dice(Dice::new(1, 1, 1))),
SignedElement::Positive(Element::Bonus(53)),
])
))