HiveBrain v1.2.0
Get Started
← Back to all entries
patternrustMinor

Lotto simulator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewsimulationstackoverflowstatisticsmathematicsrust
simulatorlottostackoverflow

Problem

With the recent craziness of the Powerball in the US, I got interested in building a little lotto simulator to see how frequently I could win with purchasing large amounts of tickets.

For those not familiar with the game and its odds and ways of winning, a Powerball ticket consists of 5 white balls between 1 and 69 and 1 red ball between 1 and 26. Below is a chart that shows the ways to win with those numbers. The left side is what you would have to match, the right side is the odds of getting that match. For example the jackpot you have to match all 5 white balls and the red ball (order doesn't matter) and the odds are 1 in 292,201,338.



Image source Kansas Lottery

A few things not taken into account in my program:

  • No 10X multiplier for power play



  • Multiple winner split of jackpot



  • Invalid characters in the input (backspacing)



Assumptions I have made in the code:

-
15% of ticket sales are with the Power Play option

-
All winnings are removed from the jackpot. I am not 100% sure where the small winnings come from in the real world, but here I am assuming they are removed from the jackpot prior to the next drawing.

-
Nothing but ticket purchases and the original 40M are added to the jackpot. I am not sure if anything is added between drawings in the real world, but here I am assuming that there is nothing.

Things I am looking for in my review:

  • Confirmation I am doing the random number generation correctly. Sometimes I feel like I am winning too frequently (less than 100M jackpots..)



  • Following best practices of either the language or general software design patterns including code styling



  • Bugs/Mistakes



  • Faster implementations



  • Clearer implementations



Bugs found:

  • The Range used in the generation of numbers was not inclusive.



I have been starting off at 250K tickets and allowing for incremental increases of the amount of ticket purchases. Typically by the time it starts taking awhile to generate tickets it has a winner. Your e

Solution

Overall, I found your code to be pretty easy to follow and understand. You stated your assumptions in your prose and most of them in the code (you could state a few more in the code too... ^_^).

-
[Good practices] You don't have any tests. That makes it very hard to make changes and be sure that the right behavior keeps happening. Because of that, all of the changes I made are only guaranteed to compile, not to actually be correct.

-
[Efficiency] The readable method is comparatively inefficient and overly broad. You force people to provide an allocated String, then build another string anyway, ignoring the allocated space you already have. You also always push to the beginning of the array, which forces moving subsequent values down, causing an O(N^2) run time. You return a String, but if you implemented something that wrote to a Write, it would be more flexible and potentially avoid some allocation.

-
[Style] You need to use indentation better. As an example:

let purchase_output = format!("Checking ticket {}/{} [{}%]", readable(x.to_string()),
readable(tickets.len().to_string()), x * 100 / tickets.len());


This is impossible to tell that it is one line unless you read it very carefully. Don't force your readers to read very carefully.

-
[Style] Use implicit returns. You should only have a return if you are exiting early from a method.

-
[Efficiency, Compatibility] Don't take a String or a Vec if you are not going to use that allocated space. Basically never accept an &String or &Vec either. Instead, accept a &str or &[T]. This means consumers can reuse allocated memory they already have, and you can accept a broader range of input types.

-
[Style] Don't use redundant type specifications:

let mut s: String = String::new();


The first String is redundant.

-
[Usability] I cannot ^C or exit your program in any easy way. Killing the program from another window leaves my terminal in a bad state.

-
[Efficiency] Allocating a String just to then append into another string is wasteful. Use write! instead to format directly into the target string.

-
[Good practices] Instead of adding a comma and then removing it, it's probably easier to just not add the comma if it's not needed.

-
[Idiomatic] There's no need for a mut keyword if the value is set only once. This includes when it is set in branches of an if statement:

let foo;
if condition {
    foo = true;
    // other stuff
} else {
    foo = false;
    // other stuff
}


-
[idiomatic] Use Rng::gen_range to sample from a range, instead of the longer way.

-
[Idiomatic] itertools is pretty great. It provides a one-line solution for readable_whites and provides a unique used elsewhere.

-
[Good practices] It's safer to use a range of 0-100 and then check if the value is

-
[Good practices] Remove all those magic numbers to constants! Especially when the value is used from multiple places (jackpot, second place!).

-
[Style] Use spaces around and inside of {} when used as a struct constructor.

-
[Good practices] Implement existing traits to be flexible. You are randomly generating a ticket, so implement Rand. This has the benefit that your code is no longer tied to thread_rng!.

-
[Good practices, Efficiency] Don't use signed integers when you have unsigned data, don't use 32 bits when 8 will do.

-
[Good practices] Don't add useless documentation. "Counts the matches between two tickets" on a method called count_matches that takes two tickets is very redundant. Describe the why not the how or what. When you have useful documentation, place it before the method and make it a doc comment (///).

-
[Good practices] There's rarely a reason to use sentinel values like -1! Use an Option or a Result instead. In this case, you could just use 0 because the winning amount is 0.

-
[Idiomatic] You can match on a tuple of things. This cleans up the base pricing cases.

-
[Bug] What happens if you have a powerplay and hit the jackpot? It looks like you will multiply the jackpot... Mistake I made while changing the code!

-
[Style] hit_jackpot doesn't need to be mutable, or even a variable.

-
[Style] In-place mutation operators like +=, *= exist. You use these some places and not others.

-
[Efficiency] You can directly push a character onto a string, no need to to_string and allocate more memory.

-
[Good practices] Short names like pp or pb are annoying to read and understand. Use real names.

-
[Style] Use an is_ prefix (or similar) for boolean variables.

-
[Bug] According to Wikipedia, Power Play can be 2×, 3×, 4×, 5×, 10×, but you have 1,2,3,4...

-
[Style] as_ref is unusual; normally you just take the reference with &.

-
[Idiomatic] Some methods seem better suited as inherent methods instead of free functions.

-
[Efficiency] Why store an array of i32 and then convert them to string? Just do it in one go.

-
[Idiomatic] Use `Str

Code Snippets

let purchase_output = format!("Checking ticket {}/{} [{}%]", readable(x.to_string()),
readable(tickets.len().to_string()), x * 100 / tickets.len());
let mut s: String = String::new();
let foo;
if condition {
    foo = true;
    // other stuff
} else {
    foo = false;
    // other stuff
}
extern crate rand;
extern crate ncurses;
extern crate postgres;
extern crate itertools;

use std::cmp::min;

use postgres::{Connection, SslMode};
use ncurses::*;
use std::char;
use rand::{Rng, Rand};
use itertools::Itertools;

struct Ticket {
    whites: Vec<u8>,
    power_ball: u8,
    is_power_play: bool,
}

// This is probably an incorrect assumption but for this simulation it will work.
const POWER_PLAY_PERCENTAGE: u8 = 15;
const WHITE_MIN: u8 = 1;
const WHITE_MAX: u8 = 69;
const POWER_BALL_MIN: u8 = 1;
const POWER_BALL_MAX: u8 = 26;

impl Rand for Ticket {
    fn rand<R: Rng>(rng: &mut R) -> Self {
        let pp_guess = rng.gen_range(0, 100);
        let pp_value = pp_guess < POWER_PLAY_PERCENTAGE;

        let mut whites_vec: Vec<_> = (0..).map(|_| rng.gen_range(WHITE_MIN, WHITE_MAX + 1)).unique().take(5).collect();
        whites_vec.sort();

        let pb_value = rng.gen_range(POWER_BALL_MIN, POWER_BALL_MAX + 1);

        Ticket { whites: whites_vec, power_ball: pb_value, is_power_play: pp_value }
    }
}

const SECOND_PLACE: i64 = 1_000_000;
const JACKPOT: i64 = 50_000_000;

impl Ticket {
    fn matching_white_numbers(&self, winner: &Ticket) -> i32 {
        let mut matching_white_numbers = 0;
        for i in 0..5 {
            for x in 0..5 {
                if self.whites[i] == winner.whites[x] {
                    matching_white_numbers += 1;
                }
            }
        }
        matching_white_numbers
    }

    fn calculate_winnings(&self, winner: &Ticket, power_play: i64) -> i64 {
        let hit_pb = self.power_ball == winner.power_ball;
        let n_matches = self.matching_white_numbers(winner);

        let mut winnings = match (n_matches, hit_pb) {
            (0, true)  => 4,
            (1, true)  => 4,
            (2, true)  => 7,
            (3, false) => 7,
            (3, true)  => 100,
            (4, false) => 100,
            (4, true)  => 50_000,
            (5, false) => SECOND_PLACE,
            (5, true)  => JACKPOT,
            _          => 0,
        };

        // Calculate power play
        if self.is_power_play {
            if winnings == SECOND_PLACE {
                winnings *= 2;
            } else {
                winnings *= power_play;
            }
        }

        winnings
    }
}

/// Goes through all tickets in this drawing assessing total winnings
fn compare_tickets(tickets: &[Ticket], winner: &Ticket) -> (i64, bool) {
    let mut winnings = 0;

    // Generate power play multiplier.
    // Normally this is done during the drawing but since the 'drawing' is just to
    // generate a ticket the same way other tickets are generated it must be done here.
    let mut rng = rand::thread_rng();
    let pp_value = rng.gen_range(1, 5);

    for (x, ticket) in tickets.iter().enumerate() {
        if x % 10_000 == 0 {
            let purchase_output = format!(
                "Checking ticket {}/{} [{}%]",
                readable(x.to_string()),
                readable(tickets.len().

Context

StackExchange Code Review Q#116399, answer score: 6

Revisions (0)

No revisions yet.