An application that compares two lists of byte strings

Question

I am a beginner in Rust, and I am trying to write a program that compares common bits between two lists of bits. The program below does that (with the variable names indicating the domain of application). I would appreciate a code review by anyone who might point out idiomatic and concise ways of accomplishing the same.

extern crate getopts;
extern crate num_bigint;
extern crate num_traits;
extern crate rand;

use num_bigint::BigUint;
use rand::distributions::{IndependentSample, Range};
use num_traits::FromPrimitive;
use std::fs;
use std::fs::File;
use std::io::Write;
use std::iter::repeat;
use getopts::Options;
use std::env;

use std::collections::HashMap;

#[derive(Debug)]
struct MyOptions {
    mutantlen: u64,
    nmutants: u64,
    ntests: u64,
    nfaults: u64,
    nchecks: u64,
    nequivalents: u64,
}

impl ToString for MyOptions {
    fn to_string(&self) -> String {
        return format!("data/mutantlen={:?}/nequivalents={:?}/nmutants={:?}/nfaults={:?}/ntests={:?}/nchecks={:?}/",
                         self.mutantlen, self.nequivalents, self.nmutants,
                         self.nfaults, self.ntests, self.nchecks);
    }
}

fn genbits(bitlen: u64, nflipped: u64) -> BigUint {
    let mut rng = rand::thread_rng();
    let faulty_bits: u64 = Range::new(1, nflipped + 1).ind_sample(&mut rng);
    let mut m: BigUint = FromPrimitive::from_usize(0).unwrap();
    for _ in 0..faulty_bits {
        let pos: usize = Range::new(0, bitlen).ind_sample(&mut rng) as usize;
        let one: BigUint = FromPrimitive::from_usize(1).unwrap();
        let fault = one << pos;
        m |= fault;
    }
    return m;
}

fn gen_lst(num: u64, len: u64, nflipped: u64) -> Vec<BigUint> {
    return (0..num).map(|_| genbits(len, nflipped)).collect(); //::<Vec<_>>
}

fn gen_mutants(nmutants: u64, mutantlen: u64, nfaults: u64) -> Vec<BigUint> {
    return gen_lst(nmutants, mutantlen, nfaults);
}

fn gen_tests(ntests: u64, mutantlen: u64, nchecks: u64) -> Vec<BigUint> {
    return gen_lst(ntests, mutantlen, nchecks);
}

fn kills(test: &BigUint, mutant: &BigUint) -> bool {
    return (test & mutant) > FromPrimitive::from_usize(0).unwrap();
}

fn zeros(size: usize) -> Vec<BigUint> {
    repeat(FromPrimitive::from_usize(0).unwrap())
        .take(size)
        .collect()
}

fn mutant_killed_by(m: &BigUint, tests: &Vec<BigUint>) -> usize {
    return tests.iter().filter(|t| kills(&t, m)).count();
}

fn mutant_killscore(
    _opts: &MyOptions,
    mutants: &Vec<BigUint>,
    equivalents: &Vec<BigUint>,
    my_tests: &Vec<BigUint>,
) -> HashMap<usize, usize> {
    return mutants.iter().chain(equivalents.iter())
        .map(|m| mutant_killed_by(m, my_tests))
        .enumerate().collect();
}

fn do_statistics(opts: &MyOptions, mutant_kills: &HashMap<usize, usize>) -> () {
    let mut ntests = Vec::new();
    for i in 0..1001 {
        let mut e = 0;
        let mut a = 0;
        let mut s = 0;
        for (_m, k) in mutant_kills {
            if *k == i {
                e += 1;
            }
            if *k >= i {
                a += 1;
            }
            if *k <= i {
                s += 1;
            }
        }
        ntests.push((i, a, s, e))
    }
    let fname = format!("{:}kills.csv", opts.to_string());
    let mut f = File::create(&fname).expect(&format!("Unable to create file: {}", &fname));

    f.write_all("ntests, atleast, atmost, exactly\n".as_bytes())
        .expect("Unable to write data");
    for &(i, a, s, e) in &ntests {
        let data = format!("{}, {}, {}, {}\n", i, a, s, e);
        f.write_all(data.as_bytes()).expect("Unable to write data");
    }
}

fn main() {
    let args: Vec<String> = env::args().map(|x| x.to_string()).collect();

    let ref _program = args[0];
    let mut opts = Options::new();
    opts.optopt("l", "mutantlen", "length of a mutant", "mutantlen");
    opts.optopt("m", "nmutants", "number of mutants", "nmutants");
    opts.optopt("t", "ntests", "number of tests", "ntests");
    opts.optopt("f", "nfaults", "maximum number of faults per mutant", "nfaults");
    opts.optopt("c", "nchecks", "maximum number of checks per test", "nchecks");
    opts.optopt("e", "nequivalents", "number of equivalents", "nequivalents");

    let matches = match opts.parse(&args[1..]) {
        Ok(m) => m,
        Err(f) => panic!(f.to_string()),
    };

    let mutantlen = match matches.opt_str("l") {
        Some(s) => s.parse().unwrap(),
        None => 10000,
    };
    let nmutants = match matches.opt_str("m") {
        Some(s) => s.parse().unwrap(),
        None => 10000,
    };
    let ntests = match matches.opt_str("t") {
        Some(s) => s.parse().unwrap(),
        None => 10000,
    };
    let nfaults = match matches.opt_str("f") {
        Some(s) => s.parse().unwrap(),
        None => 10,
    };
    let nchecks = match matches.opt_str("c") {
        Some(s) => s.parse().unwrap(),
        None => 10,
    };
    let nequivalents = match matches.opt_str("e") {
        Some(s) => s.parse().unwrap(),
        None => 0,
    };

    let opts: MyOptions = MyOptions {
        nmutants,
        mutantlen,
        nfaults,
        ntests,
        nchecks,
        nequivalents,
    };
    eprintln!("{:?}", opts);

    fs::create_dir_all(opts.to_string()).unwrap_or_else(|why| {
        println!("! {:?}", why.kind());
    });

    // first generate our tests
    let my_tests = gen_tests(ntests, mutantlen, nchecks);
    // Now generate n mutants
    let mutants = gen_mutants(nmutants, mutantlen, nfaults);

    let equivalents = zeros(nequivalents as usize);

    // how many tests killed this mutant?
    let mutant_kills = mutant_killscore(&opts, &mutants, &equivalents, &my_tests);

    do_statistics(&opts, &mutant_kills);
}

Answer 1

1. Run clippy. It will automatically tell you of a great number of improvements, such as:

   • Don't use return on the last statement.
   • You shouldn't accept a &Vec<T>.
   • You should use HashMap::values
   • let ref x = y is not idiomatic, prefer let x = &y

2. Many of your function names are very short and ndlsly_abrvd. Writing out those extra characters won't hurt.

3. Think a bit more about your code organization. It's a little surprising that you haven't created any methods and everything is currently just loose functions. For example, gen_tests, gen_mutants, and do_statistics all heavily make use of MyOptions; perhaps they should actually be methods on MyOptions instead?

MyOptions

1. It's not common to directly implement ToString. Instead, implement Display as it is more flexible (you can write it to a stream without allocating memory) and you get to_string for free (impl<T> ToString for T where T: Display + ?Sized).

2. On the flip side, it feels wrong to implement either Display or ToString for MyOptions to create a path. A similar concept exists in the standard library where a helper type is used to provide a more-obvious Display implementation.

genbits

1. BigUint implements the Zero and One traits; using them is much simpler than calling unwrap.

2. Rng::gen_range is an easier way of constructing a random range. However, Range is preferred for multiple generations, but should be hoisted out of the loop.

3. Move the numeric cast into the Range.

4. Prefer fold to avoid mutable variables and for loops.

mutant_killscore

1. Why does this take _opts? If you don't need it, don't pass it.

2. Iterator::chain will convert the argument to an iterator, you don't have to call .iter() on the argument.

do_statistics

1. Don't say a function returns -> (); just omit it entirely.

2. Instead of pushing items into a Vec, collect them using Iterator::collect

3. Are the edits / additions / subtractions really supposed to overlap? They will all be triggerred when *k == i.

4. Don't use the {:} formatter; it's the same as {}.

5. Using format! in an unwrap or expect means the string will be allocated even in success cases. Instead, use unwrap_or_else with panic.

6. In most cases, you want to use the write! / writeln! macros instead of direct calls to write_all. This also allows you to avoid allocating a string just to write it out, as you can format directly to the file.

7. There's no reason to iterate over the reference to ntests, we aren't using the vector anymore after that.

8. In fact, there's no reason to collect ntests into a Vec at all, you can iterate over it directly.

parse_arguments

1. Extract argument parsing to a new function, it outweighs the rest of the main function.

2. Don't specify the item type of a collection, use Vec<_> and let inference handle it.

3. env::args already provides Strings; there's no need to convert.

4. Don't take the reference to args[0] if you don't need it.

5. There's boilerplate in the matching of arguments, use Option::map_or instead of a match.

6. Use a closure to further reduce boilerplate between different arguments.

7. Don't redundantly declare the type of a variable (let x: Foo = Foo...).

8. Avoid having multiple authoritative sources for your values (individual variables and the MyOptions struct).

main

1. When creating the directory fails, you print a message and continue execution. That seems very suspicious.

---

extern crate getopts;
extern crate num_bigint;
extern crate num_traits;
extern crate rand;

use getopts::Options;
use num_bigint::BigUint;
use num_traits::{One, Zero};
use rand::Rng;
use rand::distributions::{IndependentSample, Range};
use std::collections::HashMap;
use std::env;
use std::fmt;
use std::fs::{self, File};
use std::io::Write;
use std::iter::repeat;

#[derive(Debug)]
struct MyOptions {
    mutantlen: u64,
    nmutants: u64,
    ntests: u64,
    nfaults: u64,
    nchecks: u64,
    nequivalents: u64,
}

impl fmt::Display for MyOptions {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "data/mutantlen={:?}/nequivalents={:?}/nmutants={:?}/nfaults={:?}/ntests={:?}/nchecks={:?}/",
            self.mutantlen,
            self.nequivalents,
            self.nmutants,
            self.nfaults,
            self.ntests,
            self.nchecks,
        )
    }
}

fn genbits(bitlen: u64, nflipped: u64) -> BigUint {
    let mut rng = rand::thread_rng();

    let faulty_bits = rng.gen_range(1, nflipped + 1);
    let rr = Range::new(0, bitlen as usize);

    (0..faulty_bits).fold(BigUint::zero(), |m, _| {
        let pos = rr.ind_sample(&mut rng);
        let fault = BigUint::one() << pos;
        m | fault
    })
}

fn gen_lst(num: u64, len: u64, nflipped: u64) -> Vec<BigUint> {
    (0..num).map(|_| genbits(len, nflipped)).collect()
}

fn gen_mutants(nmutants: u64, mutantlen: u64, nfaults: u64) -> Vec<BigUint> {
    gen_lst(nmutants, mutantlen, nfaults)
}

fn gen_tests(ntests: u64, mutantlen: u64, nchecks: u64) -> Vec<BigUint> {
    gen_lst(ntests, mutantlen, nchecks)
}

fn kills(test: &BigUint, mutant: &BigUint) -> bool {
    (test & mutant) > BigUint::zero()
}

fn zeros(size: usize) -> Vec<BigUint> {
    repeat(BigUint::zero()).take(size).collect()
}

fn mutant_killed_by(m: &BigUint, tests: &[BigUint]) -> usize {
    tests.iter().filter(|t| kills(t, m)).count()
}

fn mutant_killscore(
    mutants: &[BigUint],
    equivalents: &[BigUint],
    my_tests: &[BigUint],
) -> HashMap<usize, usize> {
    mutants
        .iter()
        .chain(equivalents)
        .map(|m| mutant_killed_by(m, my_tests))
        .enumerate()
        .collect()
}

fn do_statistics(opts: &MyOptions, mutant_kills: &HashMap<usize, usize>) {
    let ntests = (0..1001).map(|i| {
        let mut e = 0;
        let mut a = 0;
        let mut s = 0;
        for k in mutant_kills.values() {
            if *k == i {
                e += 1;
            }
            if *k >= i {
                a += 1;
            }
            if *k <= i {
                s += 1;
            }
        }
        (i, a, s, e)
    });

    let fname = format!("{}kills.csv", opts);

    let mut f = File::create(&fname).unwrap_or_else(|e| {
        panic!("Unable to create file {}: {}", &fname, e);
    });

    writeln!(f, "ntests, atleast, atmost, exactly").expect("Unable to write data");

    for (i, a, s, e) in ntests {
        writeln!(f, "{}, {}, {}, {}\n", i, a, s, e).expect("Unable to write data");
    }
}

fn parse_arguments() -> MyOptions {
    let args: Vec<_> = env::args().collect();

    let mut opts = Options::new();
    opts.optopt("l", "mutantlen", "length of a mutant", "mutantlen");
    opts.optopt("m", "nmutants", "number of mutants", "nmutants");
    opts.optopt("t", "ntests", "number of tests", "ntests");
    opts.optopt(
        "f",
        "nfaults",
        "maximum number of faults per mutant",
        "nfaults",
    );
    opts.optopt(
        "c",
        "nchecks",
        "maximum number of checks per test",
        "nchecks",
    );
    opts.optopt("e", "nequivalents", "number of equivalents", "nequivalents");

    let matches = match opts.parse(&args[1..]) {
        Ok(m) => m,
        Err(f) => panic!(f.to_string()),
    };

    let numeric_arg = |name, def| matches.opt_str(name).map_or(def, |s| s.parse().unwrap());

    MyOptions {
        nmutants: numeric_arg("m", 10_000),
        mutantlen: numeric_arg("l", 10_000),
        nfaults: numeric_arg("f", 10),
        ntests: numeric_arg("t", 10_000),
        nchecks: numeric_arg("c", 10),
        nequivalents: numeric_arg("e", 0),
    }
}

fn main() {
    let opts = parse_arguments();
    eprintln!("{:?}", opts);

    fs::create_dir_all(opts.to_string()).unwrap_or_else(|why| {
        println!("! {:?}", why.kind());
    });

    // first generate our tests
    let my_tests = gen_tests(opts.ntests, opts.mutantlen, opts.nchecks);
    // Now generate n mutants
    let mutants = gen_mutants(opts.nmutants, opts.mutantlen, opts.nfaults);

    let equivalents = zeros(opts.nequivalents as usize);

    // how many tests killed this mutant?
    let mutant_kills = mutant_killscore(&mutants, &equivalents, &my_tests);

    do_statistics(&opts, &mutant_kills);
}