Internship coding challenge post-mortem

Question

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 <[email protected]>"]
[dependencies]
rand = "0.3"

• Shepmaster: Please copy and paste your output as text. Images are rarely the correct form to communicate programming information.

Answer 1

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.

• Ameo: Thanks for the amazing feedback! Those two one-liners were really clever. I really appreciate the time you took to do this.
• Chris Morgan: The beauty of them is that they’re shorter, more efficient and clearer (after a short acclimation, anyway).