Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add D&D Character exercise #1666

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

dem4ron
Copy link
Member

@dem4ron dem4ron commented Apr 5, 2023

The exercise requires the user to roll 4 dice, so I added a test case to cover this scenario. This wasn't covered in the canonical data.

Additionally, I added another test case to confirm that the user is rolling the dice for each ability score. While there is a possibility for this test to fail, it's quite low since the probability of rolling the same score with each roll is also quite low.

Please let me know what you think about these changes

@dem4ron dem4ron requested a review from petertseng April 5, 2023 18:30
@dem4ron dem4ron changed the title Add D&D character exercise Add D&D Character exercise Apr 5, 2023
Comment on lines 20 to 46
pub fn strength(&self) -> u8 {
unimplemented!("Return the strength score of the character")
}

pub fn dexterity(&self) -> u8 {
unimplemented!("Return the dexterity score of the character")
}

pub fn constitution(&self) -> u8 {
unimplemented!("Return the constitution score of the character")
}

pub fn intelligence(&self) -> u8 {
unimplemented!("Return the intelligence score of the character")
}

pub fn wisdom(&self) -> u8 {
unimplemented!("Return the wisdom score of the character")
}

pub fn charisma(&self) -> u8 {
unimplemented!("Return the charisma score of the character")
}

pub fn hitpoints(&self) -> u8 {
unimplemented!("Return the hitpoints of the character")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly torn whether these methods are a good or a bad idea.

  • The plus: users are free to implement the struct as they see fit
  • The downside: it seems like quite boilerplatey

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true about the downside, but I think it is challenging to avoid boilerplate code in this exercise, given its classic object-oriented nature.

I personally enjoy having these getter methods, as it's allowing me to access attributes like Character.strength(). I included these methods in the stub file to ensure that the rust-analyzer doesn't show any errors when processing the test file.

on the other hand it is a perfect spot to write a macro, as I just did in the updated example file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @petertseng weigh in. The methods seem like overkill to me over regular struct access, but 🤷

Comment on lines +31 to +44
pub fn roll_four_dice() -> [u8; 4] {
let mut rng = thread_rng();
let mut rolls = [0; 4];
for i in 0..4 {
rolls[i] = rng.gen_range(1..=4)
}
rolls
}

pub fn calculate_ability_score(ability_dice_rolls: [u8; 4]) -> u8 {
let mut ability_dice_rolls = ability_dice_rolls;
ability_dice_rolls.sort();
ability_dice_rolls[1..].iter().sum()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider having just one random_ability/generate_ability/random_ability_score function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions could also be moved outside of the Character's impl, as they don't rely on the character.

Comment on lines +6 to +12
pub strength: u8,
pub dexterity: u8,
pub constitution: u8,
pub intelligence: u8,
pub wisdom: u8,
pub charisma: u8,
pub hitpoints: u8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner!

Comment on lines +133 to +134
let score = Character::calculate_ability_score(Character::roll_four_dice());
assert!((3..=18).contains(&score));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also consider adding a test that calls that function N number of times (100?) to see if it doesn't always return the same value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants