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 +80 -47
3 changed files with 80 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 3d6210b32d - Show all commits

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

@ -12,25 +12,39 @@ use std::ops::{Deref, DerefMut};
pub struct Dice { pub struct Dice {
pub(crate) count: u32, pub(crate) count: u32,
pub(crate) sides: u32, pub(crate) sides: u32,
pub(crate) keep: u32, pub(crate) keep_drop: KeepOrDrop,
pub(crate) drop: u32,
} }
impl fmt::Display for Dice { impl fmt::Display for Dice {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if self.keep != self.count { match self.keep_drop {
write!(f, "{}d{}k{}", self.count, self.sides, self.keep) KeepOrDrop::Keep(keep) => {
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 if self.drop != 0 { if keep != self.count {
write!(f, "{}d{}d{}", self.count, self.sides, self.drop) write!(f, "{}d{}k{}", self.count, self.sides, keep)
} else { } else {
write!(f, "{}d{}", self.count, self.sides) write!(f, "{}d{}", self.count, self.sides)
}
}
KeepOrDrop::Drop(drop) => {
if drop != 0 {
write!(f, "{}d{}dh{}", self.count, self.sides, drop)
} else {
write!(f, "{}d{}", self.count, self.sides)
}
}
} }
} }
} }
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum KeepOrDrop {
Keep (u32),
Drop (u32),
}
impl Dice { impl Dice {
pub fn new(count: u32, sides: u32, keep: u32, drop: u32) -> Dice { pub fn new(count: u32, sides: u32, keep_drop: KeepOrDrop) -> Dice {
Dice { count, sides, keep, drop } Dice { count, sides, keep_drop }
} }
} }

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

@ -41,7 +41,7 @@ fn parse_dice(input: &str) -> IResult<&str, Dice> {
// if ok, keep expression is present // if ok, keep expression is present
Ok(r) => (r.1.1, r.0), Ok(r) => (r.1.1, r.0),
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!
// otherwise absent and keep all dice // otherwise absent and keep all dice
Err(_) => (count, input) Err(_) => ("", input)
}; };
// check for drop expression to drop highest dice (2d20dh1) // check for drop expression to drop highest dice (2d20dh1)
@ -49,26 +49,41 @@ fn parse_dice(input: &str) -> IResult<&str, Dice> {
// if ok, keep expression is present // if ok, keep expression is present
Ok(r) => (r.1.1, r.0), Ok(r) => (r.1.1, r.0),
// otherwise absent and keep all dice // otherwise absent and keep all dice
Err(_) => ("0", input) Err(_) => ("", input)
}; };
let count: u32 = count.parse().unwrap(); let count: u32 = count.parse().unwrap();
// don't allow keep greater than number of dice, and don't allow keep zero // don't allow keep greater than number of dice, and don't allow keep zero
let mut keep: u32 = keep.parse().unwrap(); let keep_drop = match keep.parse::<u32>() {
if keep > count || keep == 0 { // Ok, there's a keep value, check and create Keep
keep = count; Ok(i) => {
} if i > count || i == 0 {
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-22 19:55:13 +00:00
Outdated
Review
Copy Link

You can make this a bit more concise with yet another match expression:

Ok(i) => match i {
    _i if _i > count || _i == 0 => KeepOrDrop::Keep(count),
    i => KeepOrDrop::Keep(i)
}

This is mostly a stylistic choice though.

You can make this a bit more concise with yet another match expression: ```rust Ok(i) => match i { _i if _i > count || _i == 0 => KeepOrDrop::Keep(count), i => KeepOrDrop::Keep(i) } ``` This is mostly a stylistic choice though.
KeepOrDrop::Keep(count)
// don't allow drop greater than or equal to number of dice } else {
let mut drop: u32 = drop.parse().unwrap(); KeepOrDrop::Keep(i)
if drop >= count { }
drop = 0; },
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-17 06:59:02 +00:00
Outdated
Review
Copy Link

Instead of unwrap, you should make use of the features of the Result type. While you will see examples of Rust code around the internet littered with unwrap, you don't actually want to use it except under a few circumstances:

  • You know the code will not crash.
  • You are okay (or don't care) with the code crashing.

In this case, might be best to combine parsing and validation:

keep.parse().map(|k| {
    //Could also use match block if it's shorter
    if k > count || k == 0 {
        count
    } else {
        k
    }
})?;

This will send up parsing errors back up the call chain and keep the boundary checks in place for a successful parse.

This comment also apples to the drop parsing.

Instead of `unwrap`, you should make use of the features of the Result type. While you will see examples of Rust code around the internet littered with unwrap, you don't actually want to use it except under a few circumstances: - You know the code will not crash. - You are okay (or don't care) with the code crashing. In this case, might be best to combine parsing and validation: ```rust keep.parse().map(|k| { //Could also use match block if it's shorter if k > count || k == 0 { count } else { k } })?; ``` This will send up parsing errors back up the call chain and keep the boundary checks in place for a successful parse. This comment also apples to the `drop` parsing.
kg333 commented 2021-09-18 02:05:31 +00:00
Outdated
Review
Copy Link

This is proving more difficult than I anticipated: it looks like nom is using its own error type? Not sure how to resolve this one, although I agree unwrap is a problem.

error[E0271]: type mismatch resolving `<u32 as std::str::FromStr>::Err == nom::Err<(&str, nom::error::ErrorKind)>`
  --> dicebot/src/basic/parser.rs:56:29
   |
56 |     let count: u32 = (count.parse())?;
   |                             ^^^^^ expected struct `ParseIntError`, found enum `nom::Err`
   |
   = note: expected struct `ParseIntError`
                found enum `nom::Err<(&str, nom::error::ErrorKind)>`
This is proving more difficult than I anticipated: it looks like nom is using its own error type? Not sure how to resolve this one, although I agree unwrap is a problem. ``` error[E0271]: type mismatch resolving `<u32 as std::str::FromStr>::Err == nom::Err<(&str, nom::error::ErrorKind)>` --> dicebot/src/basic/parser.rs:56:29 | 56 | let count: u32 = (count.parse())?; | ^^^^^ expected struct `ParseIntError`, found enum `nom::Err` | = note: expected struct `ParseIntError` found enum `nom::Err<(&str, nom::error::ErrorKind)>` ```
kg333 commented 2021-09-18 03:13:03 +00:00
Outdated
Review
Copy Link

After looking at it further, the weird error type is irrelevant. Every possible arm of the match at this point either assigns Drop or Keep, or leaves a default Keep = Count if the parse fails.

After looking at it further, the weird error type is irrelevant. Every possible arm of the match at this point either assigns Drop or Keep, or leaves a default Keep = Count if the parse fails.
} // Err, check if drop works
Err(_) => {
match drop.parse::<u32>() {
// Ok, there's a drop value, check and create Drop
Ok(i) => {
if i >= count {
KeepOrDrop::Keep(count)
} else {
KeepOrDrop::Drop(i)
}
},
// Err, there's neither keep nor drop
Err(_) => KeepOrDrop::Keep(count),
}
},
};
Ok(( Ok((
input, input,
Dice::new(count, sides.parse().unwrap(), keep, drop), Dice::new(count, sides.parse().unwrap(), keep_drop),
)) ))
} }
@ -140,29 +155,29 @@ mod tests {
use super::*; use super::*;
#[test] #[test]
fn dice_test() { fn dice_test() {
assert_eq!(parse_dice("2d4"), Ok(("", Dice::new(2, 4, 2, 0)))); assert_eq!(parse_dice("2d4"), Ok(("", Dice::new(2, 4, KeepOrDrop::Keep(2)))));
assert_eq!(parse_dice("20d40"), Ok(("", Dice::new(20, 40, 20, 0)))); assert_eq!(parse_dice("20d40"), Ok(("", Dice::new(20, 40, KeepOrDrop::Keep(20)))));
assert_eq!(parse_dice("8d7"), Ok(("", Dice::new(8, 7, 8, 0)))); assert_eq!(parse_dice("8d7"), Ok(("", Dice::new(8, 7, KeepOrDrop::Keep(8)))));
assert_eq!(parse_dice("2d20k1"), Ok(("", Dice::new(2, 20, 1, 0)))); assert_eq!(parse_dice("2d20k1"), Ok(("", Dice::new(2, 20, KeepOrDrop::Keep(1)))));
assert_eq!(parse_dice("100d10k90"), Ok(("", Dice::new(100, 10, 90, 0)))); assert_eq!(parse_dice("100d10k90"), Ok(("", Dice::new(100, 10, KeepOrDrop::Keep(90)))));
assert_eq!(parse_dice("11d10k10"), Ok(("", Dice::new(11, 10, 10, 0)))); assert_eq!(parse_dice("11d10k10"), Ok(("", Dice::new(11, 10, KeepOrDrop::Keep(10)))));
assert_eq!(parse_dice("12d10k11"), Ok(("", Dice::new(12, 10, 11, 0)))); assert_eq!(parse_dice("12d10k11"), Ok(("", Dice::new(12, 10, KeepOrDrop::Keep(11)))));
assert_eq!(parse_dice("12d10k13"), Ok(("", Dice::new(12, 10, 12, 0)))); assert_eq!(parse_dice("12d10k13"), Ok(("", Dice::new(12, 10, KeepOrDrop::Keep(12)))));
assert_eq!(parse_dice("12d10k0"), Ok(("", Dice::new(12, 10, 12, 0)))); assert_eq!(parse_dice("12d10k0"), Ok(("", Dice::new(12, 10, KeepOrDrop::Keep(12)))));
assert_eq!(parse_dice("20d40dh5"), Ok(("", Dice::new(20, 40, 20, 5)))); assert_eq!(parse_dice("20d40dh5"), Ok(("", Dice::new(20, 40, KeepOrDrop::Drop(5)))));
assert_eq!(parse_dice("8d7dh9"), Ok(("", Dice::new(8, 7, 8, 0)))); assert_eq!(parse_dice("8d7dh9"), Ok(("", Dice::new(8, 7, KeepOrDrop::Keep(8)))));
assert_eq!(parse_dice("8d7dh8"), Ok(("", Dice::new(8, 7, 8, 0)))); assert_eq!(parse_dice("8d7dh8"), Ok(("", Dice::new(8, 7, KeepOrDrop::Keep(8)))));
} }
#[test] #[test]
fn element_test() { fn element_test() {
assert_eq!( assert_eq!(
parse_element(" \t\n\r\n 8d7 \n"), parse_element(" \t\n\r\n 8d7 \n"),
Ok((" \n", Element::Dice(Dice::new(8, 7, 8, 0)))) Ok((" \n", Element::Dice(Dice::new(8, 7, KeepOrDrop::Keep(8)))))
); );
assert_eq!( assert_eq!(
parse_element(" \t\n\r\n 3d20k2 \n"), parse_element(" \t\n\r\n 3d20k2 \n"),
Ok((" \n", Element::Dice(Dice::new(3, 20, 2, 0)))) Ok((" \n", Element::Dice(Dice::new(3, 20, KeepOrDrop::Keep(2)))))
); );
assert_eq!( assert_eq!(
parse_element(" \t\n\r\n 8 \n"), parse_element(" \t\n\r\n 8 \n"),
@ -184,21 +199,21 @@ mod tests {
parse_signed_element(" \t\n\r\n- 8d4 \n"), parse_signed_element(" \t\n\r\n- 8d4 \n"),
Ok(( Ok((
" \n", " \n",
SignedElement::Negative(Element::Dice(Dice::new(8, 4, 8, 0))) SignedElement::Negative(Element::Dice(Dice::new(8, 4, KeepOrDrop::Keep(8))))
)) ))
); );
assert_eq!( assert_eq!(
parse_signed_element(" \t\n\r\n- 8d4k4 \n"), parse_signed_element(" \t\n\r\n- 8d4k4 \n"),
Ok(( Ok((
" \n", " \n",
SignedElement::Negative(Element::Dice(Dice::new(8, 4, 4, 0))) SignedElement::Negative(Element::Dice(Dice::new(8, 4, KeepOrDrop::Keep(4))))
)) ))
); );
assert_eq!( assert_eq!(
parse_signed_element(" \t\n\r\n+ 8d4 \n"), parse_signed_element(" \t\n\r\n+ 8d4 \n"),
Ok(( Ok((
" \n", " \n",
SignedElement::Positive(Element::Dice(Dice::new(8, 4, 8, 0))) SignedElement::Positive(Element::Dice(Dice::new(8, 4, KeepOrDrop::Keep(8))))
)) ))
); );
} }
@ -210,7 +225,7 @@ mod tests {
Ok(( Ok((
"", "",
ElementExpression(vec![SignedElement::Positive(Element::Dice(Dice::new( ElementExpression(vec![SignedElement::Positive(Element::Dice(Dice::new(
8, 4, 8, 0 8, 4, KeepOrDrop::Keep(8)
)))]) )))])
)) ))
); );
@ -219,7 +234,7 @@ mod tests {
Ok(( Ok((
"", "",
ElementExpression(vec![ ElementExpression(vec![
SignedElement::Positive(Element::Dice(Dice::new(2, 20, 1, 0))), SignedElement::Positive(Element::Dice(Dice::new(2, 20, KeepOrDrop::Keep(1)))),
SignedElement::Positive(Element::Bonus(5)), SignedElement::Positive(Element::Bonus(5)),
]) ])
)) ))
@ -229,7 +244,7 @@ mod tests {
Ok(( Ok((
" \n ", " \n ",
ElementExpression(vec![SignedElement::Negative(Element::Dice(Dice::new( ElementExpression(vec![SignedElement::Negative(Element::Dice(Dice::new(
8, 4, 8, 0 8, 4, KeepOrDrop::Keep(8)
)))]) )))])
)) ))
); );
@ -238,11 +253,11 @@ mod tests {
Ok(( Ok((
" 1d5 ", " 1d5 ",
ElementExpression(vec![ ElementExpression(vec![
SignedElement::Positive(Element::Dice(Dice::new(3, 4, 2, 0))), SignedElement::Positive(Element::Dice(Dice::new(3, 4, KeepOrDrop::Keep(2)))),
SignedElement::Positive(Element::Bonus(7)), SignedElement::Positive(Element::Bonus(7)),
SignedElement::Negative(Element::Bonus(5)), SignedElement::Negative(Element::Bonus(5)),
SignedElement::Negative(Element::Dice(Dice::new(6, 12, 6, 3))), SignedElement::Negative(Element::Dice(Dice::new(6, 12, KeepOrDrop::Drop(3)))),
SignedElement::Positive(Element::Dice(Dice::new(1, 1, 1, 0))), SignedElement::Positive(Element::Dice(Dice::new(1, 1, KeepOrDrop::Keep(1)))),
SignedElement::Positive(Element::Bonus(53)), SignedElement::Positive(Element::Bonus(53)),
]) ])
)) ))

6
dicebot/src/basic/roll.rs
View File

@ -4,6 +4,7 @@
* project. * project.
*/ */
use crate::basic::dice; use crate::basic::dice;
use crate::basic::dice::KeepOrDrop;
use rand::prelude::*; use rand::prelude::*;
use std::fmt; use std::fmt;
use std::ops::{Deref, DerefMut}; use std::ops::{Deref, DerefMut};
@ -86,7 +87,10 @@ impl Roll for dice::Dice {
// sort rolls in descending order // sort rolls in descending order
rolls.sort_by(|a, b| b.cmp(a)); rolls.sort_by(|a, b| b.cmp(a));
DiceRoll(rolls,self.keep as usize, self.drop as usize) match self.keep_drop {
KeepOrDrop::Keep(k) => DiceRoll(rolls,k as usize, 0),
KeepOrDrop::Drop(dh) => DiceRoll(rolls,self.count as usize, dh as usize),
}
} }
} }