7
\$\begingroup\$

Introduction

I decided to get my feet wet in Rust by going ahead an implementing a full crate with the tests, documentation, and all other accompanying stuff. This is a toy implementation of a library containing a function that checks whether a given u64 is prime or not.

What I've also done is implement a main.rs in addition to the lib.rs, that produces an executable when the cargo run or cargo build command is issued. So far the toolchain hasn't complained yet. It seems to be working fine, but I seek adivce on the practicality of it.

The organization

Here is the full structure (excluding the target folder):

prime
+- src
|  +- lib.rs
|  +- main.rs
|  +- prime.rs
+- tests
|  +- extern_test.rs
+- Cargo.lock
+- Cargo.toml

The files

lib.rs

pub mod prime;

main.rs

pub mod prime;

use std::env;
use std::io::stdin;

fn take_input() {
    println!("Prime cheker utility.\n=====================\n");
    loop {
        process_single_line();
        if user_wants_to_exit() {
            break;
        }
    }
}

fn process_single_line() {
    let mut num_str: String = String::new();

    println!("Enter the number to check : ");

    stdin().read_line(&mut num_str).unwrap();

    process_string(num_str.trim());
}

fn user_wants_to_exit() -> bool {
    let mut usr_str = String::new();

    println!("Do you want to exit? (y/n) : ");
    stdin()
        .read_line(&mut usr_str)
        .expect("Error while reading input.");

    let trimmed = usr_str.trim();

    trimmed == "y" || trimmed == "Y" || trimmed.to_lowercase() == "yes"
}

fn process_string(num_str: &str) {
    let num = num_str.parse::<u64>().expect(INVALID_NUMBER);

    println!(
        "The integer {} is{} a prime.",
        num,
        match prime::is_prime(num) {
            true => "",
            false => " not",
        }
    );
}

const HELP_TEXT: &str = "USAGE:\n\n1. prime\n2. prime [unsigned integer]\n";
const INVALID_NUMBER: &str = "Please enter a valid unsigned integer.";

fn main() {
    let args: Vec<String> = env::args().collect();
    match args.len() {
        1 => take_input(),
        2 => process_string(args[1].trim()),
        _ => {
            println!("{}", HELP_TEXT);
        }
    }
}

prime.rs

/// This function takes a 64-bit unsigned integer and checks if it is a prime.
///
/// If the number is prime, `true` is returned, and vice-versa.
///
/// #Example
///
/// ```rust
/// use prime_util::*;
///
/// let result = prime::is_prime(31);
/// assert_eq!(result, true);
/// ```
pub fn is_prime(num: u64) -> bool {
    if num < 2 {
        return false;
    }
    if num == 2 || num == 3 {
        return true;
    }
    // Even numbers and multiples of 3 are eliminated
    if num % 2 == 0 || num % 3 == 0 {
        return false;
    }

    // Optimized divisor approach
    // First we calculate the maximum limit of iteration
    let limit = (num as f64).sqrt() as u64;
    // We start the iteration from 5 (2 and 3 have been already tested)
    let mut divisor = 5;
    // The step alternates between 2 and 4 to keep the divisor of the form
    // 6k +/- 1, where k is an integer
    let mut step = 2;
    while divisor <= limit {
        if num % divisor == 0 {
            return false;
        }
        divisor += step;
        step = if step == 2 { 4 } else { 2 }
    }
    true
}

extern_test.rs

extern crate prime_util;

#[cfg(test)]
mod integration_test {
    use prime_util::prime;
    #[test]
    fn small_number_checks() {
        assert_eq!(prime::is_prime(11), true);
        assert_eq!(prime::is_prime(12), false);
        assert_eq!(prime::is_prime(13), true);
    }

    #[test]
    fn large_number_checks() {
        assert_eq!(prime::is_prime(179434027), true);
        assert_eq!(prime::is_prime(179434029), false);
        assert_eq!(prime::is_prime(179434031), false);
        assert_eq!(prime::is_prime(179434033), true);
    }

    #[test]
    fn first_10_numbers_tested() {
        assert_eq!(prime::is_prime(0), false);
        assert_eq!(prime::is_prime(1), false);
        assert_eq!(prime::is_prime(2), true);
        assert_eq!(prime::is_prime(3), true);
        assert_eq!(prime::is_prime(4), false);
        assert_eq!(prime::is_prime(5), true);
        assert_eq!(prime::is_prime(6), false);
        assert_eq!(prime::is_prime(7), true);
        assert_eq!(prime::is_prime(8), false);
        assert_eq!(prime::is_prime(9), false);
        assert_eq!(prime::is_prime(10), false);
    }
}

Others

Cargo.toml is the default one. Nothing has been changed.

Topics of interest

I would like to point out some specific areas that I would like to hear advice on (although I always welcome comments on any aspect of the project):

  1. The fact that I have both main.rs and lib.rs. Cargo couldn't care less apparently. Because it executes all tests (even the documentation test) and also executes as a command-line program when cargo run is called (both with and without arguments).
  2. The general quality of the code. I feel it can always be improved and made more Rusty.
  3. The organization of the files and the containing code, and the documentation.
  4. The external tests that I've included. Any additions/modifications necessary?

I reiterate my invitation for general criticism as well.

\$\endgroup\$
0

1 Answer 1

5
\$\begingroup\$

First, instead of having your main in the root of src I would organize the project like this:

prime
+- src
|  +- bin
   |  +- main.rs
|  +- lib.rs
|  +- prime.rs
+- tests
|  +- extern_test.rs
+- Cargo.lock
+- Cargo.toml

In your main.rs you would use your own crate like any other external crate (assuming the name of your lib is "prime_util" as it looks from your test file:

main.rs

extern crate prime_util;

use std::env;
use std::io::stdin;

...

Regarding code quality in general there are some un-idiomatic things there but nothing very serious, and definately some things that are just a matter of taste, like this part where you have a match statement as a parameter:

println!(
    "The integer {} is{} a prime.",
    num,
    match prime::is_prime(num) {
        true => "",
        false => " not",
    }
);

I find that a bit hard to follow, and would either construct a string or have two different print statements based on the match, but I would say this is more a matter of taste than right/wrong.

One thing that is something I really would look into is error handling. Your project is rather small and the only place your code can produce errors is in the application, and I would say it's OK to unwrap() or expect() there if you really mean that the error is unexpected and puts the program in an invalid state, but if you want the code to be as good as possible you would handle more error cases.

I.e. If the user accidentally wrote o instead of 0 should the program really panic?

Here's a small example with simple error handling, where you implement the simplest of error types with an enum. When you scale up you will see that this will make error handling quite a bit easier and the program much more robust:

When you start working through like this you will also see that there are challenges with how you've split up the functions between main and take_input.

But this is my opinion so take it for what it's worth. Hope it helps anyway :)

Main.rs (with some simple error handling)

pub mod prime;

use std::env;
use std::io::stdin;
fn take_input() {
    println!("Prime cheker utility.\n=====================\n");
    loop {
        match process_single_line() {
            Err(e) => match e {
                MyError::InvalidDigit(msg) => {
                    println!("{}", msg);
                    println!("Please try again");
                },
                _ => break,
            },
            Ok(_) => (),
        }
        if user_wants_to_exit() {
            break;
        }
    }
}

#[derive(Debug)]
enum MyError {
    InvalidDigit(String),
    // place your different errors here
    Other,
}

use std::fmt;
impl fmt::Display for MyError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            MyError::InvalidDigit(msg) => write!(f, "{}", msg),
            MyError::Other => write!(f, "Unexpected error"),
        }
    }
}

impl std::error::Error for MyError {}

fn process_single_line() -> Result<(), MyError> {
    let mut num_str: String = String::new();


    println!("Enter the number to check : ");

    stdin().read_line(&mut num_str).unwrap();

    process_string(num_str.trim())
}

fn user_wants_to_exit() -> bool {
    let mut usr_str = String::new();

    println!("Do you want to exit? (y/n) : ");
    stdin()
        .read_line(&mut usr_str)
        .expect("Error while reading input.");

    let trimmed = usr_str.trim();

    trimmed == "y" || trimmed == "Y" || trimmed.to_lowercase() == "yes"
}

fn process_string(num_str: &str) -> Result<(), MyError>  {
    let num = num_str.parse::<u64>()
    .map_err(|_| MyError::InvalidDigit(format!("\"{}\" is not a valid digit.", num_str)))?;

    println!(
        "The integer {} is{} a prime.",
        num,
        match prime::is_prime(num) {
            true => "",
            false => " not",
        }
    );
    Ok(())
}

const HELP_TEXT: &str = "USAGE:\n\n1. prime\n2. prime [unsigned integer]\n";
const INVALID_NUMBER: &str = "Please enter a valid unsigned integer.";

fn main() {
    let args: Vec<String> = env::args().collect();
    match args.len() {
        1 => take_input(),
        2 => {
            process_string(args[1].trim()).ok();
            },
        _ => {
            println!("{}", HELP_TEXT);
        }
    }
}

Update

I created this example repository with some quick changes to project structure and code (mainly focusing on main.rs) to show some of the ideas I mentioned above.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for a really great answer! I'd really appreciate if you would illustrate how to write idiomatic code in this instance, for it would help me modify my style accordingly. Also, if it's not too much trouble, could you perhaps demonstrate how you would have split up the functions (or not at all?). \$\endgroup\$ Commented Dec 27, 2018 at 14:03
  • \$\begingroup\$ Shure. It's easier for me to just show you in a repo. See my updated asnwer or go directly to it here: github.com/cfsamson/examples_codereview_prime \$\endgroup\$
    – cfs
    Commented Dec 27, 2018 at 21:21
  • 1
    \$\begingroup\$ I'll have to mention that I've spent limited time with this but I hope it shows some of the things I talked about above. That said. In a binary like this for only my own use I would unwrap() and not do proper error handling if it wasn't important if it's just a quick tool for me. But if I make it for someone else to use, and want it to be easy to expand later I would start out the way I show you here. \$\endgroup\$
    – cfs
    Commented Dec 27, 2018 at 21:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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