2
\$\begingroup\$

I wrote this in 78 minutes as part of an application to an internship program. We were allowed to use whatever language we wanted, so I picked Rust because that's what I use the most. I'm a Sophomore CS/Data Science major and I just wanted to get a feel for how you more experienced among us feel about what I made.

I'm not asking for detailed analysis (I know it works and that that it fits the prompt), I'm just looking for broad feedback and any random observations.

Prompt TL;DR:

  • server picks a random number

  • clients try to guess it

  • if client guesses it, server raises their balance by 1 and picks new number


//! XternCoin Application
//! Casey Primozic - 2016
//!
//! Requires latest Rust nightly

// turn on the unstable testing feature
#![feature(test)]

extern crate rand;
extern crate test;

use std::sync::{Arc, Mutex};
use rand::{thread_rng, Rng};
use std::collections::HashMap;
use std::sync::atomic::{Ordering, AtomicUsize};
use std::thread;
use std::convert::From;

/// The server which manages the Proof of Work check and dispenses the currency
///
/// The server is threadsafe and can be duplicated over an arbitary number of threads
/// so that any number of users can mine simultaneously.
#[derive(Clone)]
struct CoinServer {
    // concurrent hashmap to store the balances of all users
    // this allows shared mutable access to the balance database through `lock()`.
    pub balances: Arc<Mutex<HashMap<String, f64>>>, // only public for sake of the test
    pub random_num: Arc<AtomicUsize>,
}

impl CoinServer {
    /// Function which takes a user's id and a user's guess,
    /// and returns whether or not their guess was correct.
    pub fn handle_guess(&mut self, user_id: String, guess: u64) {
        // convert the String to a u64 for comparison
        if self.random_num.load(Ordering::Relaxed) == guess as usize {
            self.inc_balance(user_id);
            self.get_new_rand();
        }
    }

    /// Adds one to the user's balance, creating an entry for it if
    /// it doesn't exist.
    fn inc_balance(&mut self, user_id: String) {
        // lock the balances
        let mut balances = self.balances.lock().unwrap();

        // insert user if not already in database
        if !balances.contains_key(&user_id) {
            balances.insert(user_id, 1f64);
            return
        }

        // credit the user for the correct guess
        let balance = balances.get_mut(&user_id).unwrap();
        *balance += 1f64;
    }

    /// Function which takes a userid and returns
    /// how many coins they have.
    pub fn get_coins(&mut self, user_id: String) -> f64 {
        let balances = self.balances.lock().unwrap();

        if !balances.contains_key(&user_id) { return 0f64 }
        *balances.get(&user_id).unwrap()
    }

    pub fn new() -> CoinServer {
        let mut server = CoinServer {
            balances: Arc::new(Mutex::new(HashMap::new())),
            random_num: Arc::new(AtomicUsize::new(0))
        };
        server.get_new_rand();
        server
    } // I'm using a text editor to debug this: https://ameo.link/u/3ew.png

    /// Creates a new random number for users to try to guess
    fn get_new_rand(&mut self) {
        // entropy source
        let mut rng = rand::thread_rng();
        // generate random number from 0 to 100000
        let rand_num: usize = rng.gen_range(0, 100000);
        self.random_num.store(rand_num, Ordering::Relaxed);
    }
}


/// A function which, when called, pretends to be a user of
/// XternCoin and uses the other two functions you've written
/// to accumulate coins by guessing random numbers in a loop
fn start_guessing(user_id: String, mut server: CoinServer, iterations: usize) {
    // entropy source
    let mut rng = rand::thread_rng();

    for _ in 0..iterations {
        let guess: u64 = rng.gen_range(0, 100000);
        // make guess and let server verify it
        server.handle_guess(user_id.clone(), guess);
    }
}

fn main() {
    let server = CoinServer::new();

    // spawn 10 threads to start guessing
    for i in 0..10 {
        let mut server_clone = server.clone();
        thread::spawn(move || {
            let user_id = format!("{}", i);
            // initiate mining
            start_guessing(user_id.clone(), server_clone.clone(), 100000);
            println!("Balance for {} after this mining session: {}", user_id.clone(), server_clone.get_coins(user_id));
        });
    }

    // block so the miners can mine
    thread::park();
}

// TODO: tests1

/// Check to make sure that user balances are created and incremented
#[test]
fn test_mining() {
    let mut server = CoinServer {
        balances: Arc::new(Mutex::new(HashMap::new())),
        random_num: Arc::new(AtomicUsize::new(42)) // cheat for the test
    };

    let user_id = "Test".to_string();
    server.handle_guess(user_id.clone(), 42);

    // make sure we got credited
    assert_eq!(server.get_coins(user_id), 1f64);
    // server generated a new random number
    assert!(server.random_num.load(Ordering::Relaxed) != 42);
    // test passes: https://ameo.link/u/3ey.png
}

Program output

Cargo.toml:

[package]
name = "xt"
version = "0.1.0"
authors = ["Casey Primozic <me@ameo.link>"]

[dependencies]
rand = "0.3"
\$\endgroup\$
  • 2
    \$\begingroup\$ Please copy and paste your output as text. Images are rarely the correct form to communicate programming information. \$\endgroup\$ – Shepmaster Oct 26 '16 at 12:09
4
\$\begingroup\$
  1. Making private fields public only for the sake of tests is undesirable; you should instead add new methods which are only available in tests, e.g.

    #[cfg(test)]
    fn get_random_number(&self) -> usize {
        self.random_num.load(Ordering::Relaxed)
    }
    

    Expressed simply: testing your own code shouldn’t have any impact on the API exposed to library users.

  2. AtomicUsize is already Sync; no need to wrap it in Arc.

  3. user_id should be &str rather than String in most places. Basically, you should never need to clone the String after the first time when you insert it into balances (to “remedy” that, you could use Arc<String> instead if you wished; not sure if I would or not). You’re doing a lot more memory allocation than is necessary.

  4. Unwrapping is undesirable. See if you can avoid it. get_coins can be rewritten thus (note how this also makes it only one lookup rather than two, so it’s faster as well):

    pub fn get_coins(&mut self, user_id: String) -> f64 {
        *self.balances.lock().unwrap().get(&user_id).unwrap_or(&0)
    }
    
  5. HashMap has some really nice things for efficiency. Just as get returned an Option in the previous point (which rendered the contains_key part superfluous), inc_balance can use the Entry API to do less work:

    fn inc_balance(&mut self, user_id: String) {
        // Credit the user for the correct guess,
        // inserting the user if not already in the database
        *self.balances.lock().unwrap().entry(user_id).or_insert(0) += 1;
    }
    

    (Note: at present this actually doesn’t play optimally with using &str for user_id as it requires you to clone the user ID every time you call inc_balance rather than only the first time a user ID is encountered. RFC PR 1769 would fix that.)

  6. use std::convert::From; is unnecessary (From is in the prelude).

  7. #![feature(test)] shouldn’t be necessary; you don’t appear to be using any unstable features (mostly just benchmarking, really).

  8. Something to bear in mind: you’re locking the entire balances table to make any changes at present. This is probably undesirable. If you were doing this seriously, you’d be using a proper database which would take care of this stuff properly. Just thought I’d mention it.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the amazing feedback! Those two one-liners were really clever. I really appreciate the time you took to do this. \$\endgroup\$ – Ameo Oct 27 '16 at 3:16
  • 2
    \$\begingroup\$ The beauty of them is that they’re shorter, more efficient and clearer (after a short acclimation, anyway). \$\endgroup\$ – Chris Morgan Oct 27 '16 at 12:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.