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

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

@ -13,21 +13,24 @@ pub struct Dice {
pub(crate) count: u32,
pub(crate) sides: u32,
pub(crate) keep: u32,
pub(crate) drop: 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)
} else {
if self.keep != self.count {
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.
write!(f, "{}d{}k{}", self.count, self.sides, self.keep)
} else if self.drop != 0 {
write!(f, "{}d{}d{}", self.count, self.sides, self.drop)
} else {
write!(f, "{}d{}", self.count, self.sides)
}
}
}
impl Dice {
pub fn new(count: u32, sides: u32, keep: u32) -> Dice {
Dice { count, sides, keep }
pub fn new(count: u32, sides: u32, keep: u32, drop: u32) -> Dice {
Dice { count, sides, keep, drop }
}
}

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

@ -35,6 +35,7 @@ enum Sign {
fn parse_dice(input: &str) -> IResult<&str, Dice> {
// parse main dice expression
let (mut input, (count, _, sides)) = tuple((digit1, tag("d"), digit1))(input)?;
// check for keep expression (i.e. D&D 5E Advantage)
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.
let keep;
match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) {
@ -46,16 +47,36 @@ fn parse_dice(input: &str) -> IResult<&str, Dice> {
// otherwise absent and keep all dice
Err(_) => keep = count,
};
// check for drop expression (i.e. D&D 5E Disadvantage)
let drop;
match tuple::<&str, _, (_, _), _>((tag("d"), digit1))(input) {
// if ok, drop expression is present
Ok(r) => {
input = r.0;
drop = r.1.1;
}
// otherwise absent and keep all dice
Err(_) => drop = "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.
let count: u32 = count.parse().unwrap();
// don't allow keep greater than number of dice, and don't allow keep zero
let mut keep: u32 = keep.parse().unwrap();
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.
let count: u32 = count.parse().unwrap();
if keep > count || keep == 0 {
keep = count;
}
// don't allow drop greater than or equal to number of dice
let mut drop: u32 = drop.parse().unwrap();
if drop >= count {
drop = 0;
}
Ok((
input,
Dice::new(count, sides.parse().unwrap(), keep),
Dice::new(count, sides.parse().unwrap(), keep, drop),
))
}
@ -127,26 +148,29 @@ mod tests {
use super::*;
#[test]
fn dice_test() {
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))));
assert_eq!(parse_dice("2d20k1"), Ok(("", Dice::new(2, 20, 1))));
assert_eq!(parse_dice("100d10k90"), Ok(("", Dice::new(100, 10, 90))));
assert_eq!(parse_dice("11d10k10"), Ok(("", Dice::new(11, 10, 10))));
assert_eq!(parse_dice("12d10k11"), Ok(("", Dice::new(12, 10, 11))));
assert_eq!(parse_dice("12d10k13"), Ok(("", Dice::new(12, 10, 12))));
assert_eq!(parse_dice("12d10k0"), Ok(("", Dice::new(12, 10, 12))));
assert_eq!(parse_dice("2d4"), Ok(("", Dice::new(2, 4, 2, 0))));
assert_eq!(parse_dice("20d40"), Ok(("", Dice::new(20, 40, 20, 0))));
assert_eq!(parse_dice("8d7"), Ok(("", Dice::new(8, 7, 8, 0))));
assert_eq!(parse_dice("2d20k1"), Ok(("", Dice::new(2, 20, 1, 0))));
assert_eq!(parse_dice("100d10k90"), Ok(("", Dice::new(100, 10, 90, 0))));
assert_eq!(parse_dice("11d10k10"), Ok(("", Dice::new(11, 10, 10, 0))));
assert_eq!(parse_dice("12d10k11"), Ok(("", Dice::new(12, 10, 11, 0))));
assert_eq!(parse_dice("12d10k13"), Ok(("", Dice::new(12, 10, 12, 0))));
assert_eq!(parse_dice("12d10k0"), Ok(("", Dice::new(12, 10, 12, 0))));
assert_eq!(parse_dice("20d40d5"), Ok(("", Dice::new(20, 40, 20, 5))));
assert_eq!(parse_dice("8d7d9"), Ok(("", Dice::new(8, 7, 8, 0))));
assert_eq!(parse_dice("8d7d8"), Ok(("", Dice::new(8, 7, 8, 0))));
}
#[test]
fn element_test() {
assert_eq!(
parse_element(" \t\n\r\n 8d7 \n"),
Ok((" \n", Element::Dice(Dice::new(8, 7, 8))))
Ok((" \n", Element::Dice(Dice::new(8, 7, 8, 0))))
);
assert_eq!(
parse_element(" \t\n\r\n 3d20k2 \n"),
Ok((" \n", Element::Dice(Dice::new(3, 20, 2))))
Ok((" \n", Element::Dice(Dice::new(3, 20, 2, 0))))
);
assert_eq!(
parse_element(" \t\n\r\n 8 \n"),
@ -168,21 +192,21 @@ mod tests {
parse_signed_element(" \t\n\r\n- 8d4 \n"),
Ok((
" \n",
SignedElement::Negative(Element::Dice(Dice::new(8, 4, 8)))
SignedElement::Negative(Element::Dice(Dice::new(8, 4, 8, 0)))
))
);
assert_eq!(
parse_signed_element(" \t\n\r\n- 8d4k4 \n"),
Ok((
" \n",
SignedElement::Negative(Element::Dice(Dice::new(8, 4, 4)))
SignedElement::Negative(Element::Dice(Dice::new(8, 4, 4, 0)))
))
);
assert_eq!(
parse_signed_element(" \t\n\r\n+ 8d4 \n"),
Ok((
" \n",
SignedElement::Positive(Element::Dice(Dice::new(8, 4, 8)))
SignedElement::Positive(Element::Dice(Dice::new(8, 4, 8, 0)))
))
);
}
@ -194,7 +218,7 @@ mod tests {
Ok((
"",
ElementExpression(vec![SignedElement::Positive(Element::Dice(Dice::new(
8, 4, 8
8, 4, 8, 0
)))])
))
);
@ -203,7 +227,7 @@ mod tests {
Ok((
"",
ElementExpression(vec![
SignedElement::Positive(Element::Dice(Dice::new(2, 20, 1))),
SignedElement::Positive(Element::Dice(Dice::new(2, 20, 1, 0))),
SignedElement::Positive(Element::Bonus(5)),
])
))
@ -213,20 +237,20 @@ mod tests {
Ok((
" \n ",
ElementExpression(vec![SignedElement::Negative(Element::Dice(Dice::new(
8, 4, 8
8, 4, 8, 0
)))])
))
);
assert_eq!(
parse_element_expression("\t3d4 + 7 - 5 - 6d12 + 1d1 + 53 1d5 "),
parse_element_expression("\t3d4k2 + 7 - 5 - 6d12d3 + 1d1 + 53 1d5 "),
Ok((
" 1d5 ",
ElementExpression(vec![
SignedElement::Positive(Element::Dice(Dice::new(3, 4, 3))),
SignedElement::Positive(Element::Dice(Dice::new(3, 4, 2, 0))),
SignedElement::Positive(Element::Bonus(7)),
SignedElement::Negative(Element::Bonus(5)),
SignedElement::Negative(Element::Dice(Dice::new(6, 12, 6))),
SignedElement::Positive(Element::Dice(Dice::new(1, 1, 1))),
SignedElement::Negative(Element::Dice(Dice::new(6, 12, 6, 3))),
SignedElement::Positive(Element::Dice(Dice::new(1, 1, 1, 0))),
SignedElement::Positive(Element::Bonus(53)),
])
))

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

@ -19,8 +19,8 @@ pub trait Rolled {
}
#[derive(Debug, PartialEq, Eq, Clone)]
// array of rolls in order, and how many dice to keep
pub struct DiceRoll (pub Vec<u32>, usize);
// array of rolls in order, how many dice to keep, and how many to drop
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-17 07:01:48 +00:00
Outdated
Review
Copy Link

Add a third / to get the Rustdoc working. It may also be good to more thoroughly describe what keep and drop mean here, namely that they mean keeping the highest or dropping the highest rolls.

Add a third `/` to get the Rustdoc working. It may also be good to more thoroughly describe what keep and drop mean here, namely that they mean keeping the highest or dropping the highest rolls.
pub struct DiceRoll (pub Vec<u32>, usize, usize);
impl DiceRoll {
pub fn rolls(&self) -> &[u32] {
@ -31,9 +31,13 @@ impl DiceRoll {
self.1
}
pub fn drop(&self) -> usize {
self.2
}
// only count kept dice in total
pub fn total(&self) -> u32 {
self.0[..self.1].iter().sum()
self.0[self.2..self.1].iter().sum()
}
}
@ -48,14 +52,19 @@ impl fmt::Display for DiceRoll {
write!(f, "{}", self.rolled_value())?;
let rolls = self.rolls();
let keep = self.keep();
let drop = self.drop();
let mut iter = rolls.iter().enumerate();
if let Some(first) = iter.next() {
write!(f, " ({}", first.1)?;
if drop != 0 {
write!(f, " ([{}]", first.1)?;
} else {
write!(f, " ({}", first.1)?;
}
for roll in iter {
if roll.0 < keep {
write!(f, " + {}", roll.1)?;
} else {
if roll.0 >= keep || roll.0 < drop {
write!(f, " + [{}]", roll.1)?;
} else {
write!(f, " + {}", roll.1)?;
}
}
write!(f, ")")?;
@ -75,7 +84,7 @@ impl Roll for dice::Dice {
// sort rolls in descending order
rolls.sort_by(|a, b| b.cmp(a));
DiceRoll(rolls,self.keep as usize)
DiceRoll(rolls,self.keep as usize, self.drop as usize)
}
}
@ -211,22 +220,26 @@ mod tests {
use super::*;
#[test]
fn dice_roll_display_test() {
assert_eq!(DiceRoll(vec![1, 3, 4], 3).to_string(), "8 (1 + 3 + 4)");
assert_eq!(DiceRoll(vec![], 0).to_string(), "0");
assert_eq!(DiceRoll(vec![1, 3, 4], 3, 0).to_string(), "8 (1 + 3 + 4)");
assert_eq!(DiceRoll(vec![], 0, 0).to_string(), "0");
assert_eq!(
DiceRoll(vec![4, 7, 2, 10], 4).to_string(),
DiceRoll(vec![4, 7, 2, 10], 4, 0).to_string(),
"23 (4 + 7 + 2 + 10)"
);
assert_eq!(
DiceRoll(vec![20, 13, 11, 10], 3).to_string(),
DiceRoll(vec![20, 13, 11, 10], 3, 0).to_string(),
"44 (20 + 13 + 11 + [10])"
);
assert_eq!(
DiceRoll(vec![20, 13, 11, 10], 4, 1).to_string(),
"34 ([20] + 13 + 11 + 10)"
);
}
#[test]
fn element_roll_display_test() {
assert_eq!(
ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3)).to_string(),
ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3, 0)).to_string(),
"8 (1 + 3 + 4)"
);
assert_eq!(ElementRoll::Bonus(7).to_string(), "7");
@ -235,11 +248,11 @@ mod tests {
#[test]
fn signed_element_roll_display_test() {
assert_eq!(
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3))).to_string(),
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3, 0))).to_string(),
"8 (1 + 3 + 4)"
);
assert_eq!(
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3))).to_string(),
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3, 0))).to_string(),
"-8 (1 + 3 + 4)"
);
assert_eq!(
@ -256,14 +269,14 @@ mod tests {
fn element_expression_roll_display_test() {
assert_eq!(
ElementExpressionRoll(vec![SignedElementRoll::Positive(ElementRoll::Dice(
DiceRoll(vec![1, 3, 4], 3)
DiceRoll(vec![1, 3, 4], 3, 0)
)),])
.to_string(),
"8 (1 + 3 + 4)"
);
assert_eq!(
ElementExpressionRoll(vec![SignedElementRoll::Negative(ElementRoll::Dice(
DiceRoll(vec![1, 3, 4], 3)
DiceRoll(vec![1, 3, 4], 3, 0)
)),])
.to_string(),
"-8 (1 + 3 + 4)"
@ -280,8 +293,8 @@ mod tests {
);
assert_eq!(
ElementExpressionRoll(vec![
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3))),
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![1, 2], 2))),
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3, 0))),
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![1, 2], 2, 0))),
SignedElementRoll::Positive(ElementRoll::Bonus(4)),
SignedElementRoll::Negative(ElementRoll::Bonus(7)),
])
@ -290,8 +303,8 @@ mod tests {
);
assert_eq!(
ElementExpressionRoll(vec![
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3))),
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![1, 2], 2))),
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![1, 3, 4], 3, 0))),
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![1, 2], 2, 0))),
SignedElementRoll::Negative(ElementRoll::Bonus(4)),
SignedElementRoll::Positive(ElementRoll::Bonus(7)),
])
@ -300,13 +313,23 @@ mod tests {
);
assert_eq!(
ElementExpressionRoll(vec![
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![4, 3, 1], 3))),
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![12, 2], 1))),
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![4, 3, 1], 3, 0))),
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![12, 2], 1, 0))),
SignedElementRoll::Negative(ElementRoll::Bonus(4)),
SignedElementRoll::Positive(ElementRoll::Bonus(7)),
])
.to_string(),
"7 (-8 (4 + 3 + 1) + 12 (12 + [2]) - 4 + 7)"
);
assert_eq!(
ElementExpressionRoll(vec![
SignedElementRoll::Negative(ElementRoll::Dice(DiceRoll(vec![4, 3, 1], 3, 1))),
SignedElementRoll::Positive(ElementRoll::Dice(DiceRoll(vec![12, 2], 2, 0))),
SignedElementRoll::Negative(ElementRoll::Bonus(4)),
SignedElementRoll::Positive(ElementRoll::Bonus(7)),
])
.to_string(),
"13 (-4 ([4] + 3 + 1) + 14 (12 + 2) - 4 + 7)"
);
}
}