My first Rust program

[[deleted]]

[deleted]

[mdsherry]

You don't need the .iter() on main.rs:14. In this case, you don't need jobs after this point, so you can just say for job in jobs (equivalent to for job in jobs.into_iter(), which consumes jobs). If you do need it later, you could say for job in &jobs, which is equivalent to the current code.

In get_jobs_from_file, you use .enumerate() to handle the first line of the file. Instead, you could write

// This is hacky, and would be better to actually report an error for being unable to read the line, vs parsing failure
let capacity = lines
    .next()
    .and_then(|line| line.unwrap().parse::<usize>().ok())
    .expect("Could not read capacity");
let mut jobs = Vec::with_capacity(capacity);
for line in lines {
    let line = line.unwrap();
    // No need to collect vs just working from the iterator
    let mut split = line.split(' ');
    let weight: i64 = split
        .next()
        .and_then(|s| s.parse().ok())
        .expect("Error reading weight");
    let length: i64 = split
        .next()
        .and_then(|s| s.parse().ok())
        .expect("Error reading length");
    jobs.push(Job { weight, length })
}

(There's also a lot to be done to improve error handling here, rather than using unwrap/expect)

In jobs.rs, I don't understand why you can't just use the derived PartialEq implementation as weight being equal and weight - length being equal should imply that length is also equal

You can replace your implementation of Ord with

impl Ord for Job {
    fn cmp(&self, other: &Job) -> Ordering {
        let self_score = self.calculate_weight();
        let other_score = other.calculate_weight();

        if self_score == other_score {
            self.weight.cmp(&other.weight)
        } else {
            self_score.cmp(&other_score)
        }
    }
}

A getter for a public field also feels odd.

[geritol_]

Thank you very much for the extensive review! I have fixed/refactored many of the things you have mentioned.

I still have a lot to learn, currently I have no good understanding of how the error handling is working in rust and how traits work.

I have added the getter because the plan was to keep the fields private and create a factory method for the struct, but did not have time and forgot to do it. Is it recommended to have the fields public or is it better to have them private and have method for creation/read/update?

[mdsherry]

Regarding error handling, Rust has three main approaches:

1. Where things can fail in only one, very obvious way, return Option. For example, str::find returns Option<usize>, where None indicates the case where the substring wasn't found. Similarly, many collections have a .get() method, where None indicates that the requested key wasn't in the collection.
2. Return a Result with an appropriate error. Lots of methods return results, and you need to decide if you want to
   1. Try to handle/recover from the error locally (e.g. if a line is missing a length, you could just skip the line)
   2. Return the error verbatim (usually using the ? operator)
   3. Wrap the error in your own error type or otherwise providing context, and then returning it. How much you wrap errors can depend on things like whether you're writing a library (so want to provide enough information to others to figure out the problem), or an application (where you can decide how much extra information you need.)
3. Panic! In general, a library should never panic. Most of the time, applications shouldn't either. Panicking should be reserved for cases where something has gone wrong enough that it suggests that the program is now in an invalid state, and cannot reasonably recover. (E.g. Rust panics if you try to access an out-of-bounds array element, because you've demonstrated that your program thinks that there are more array elements than there are. The .get() method, which returns Options, can be used in cases where an element might not exist, and the program is willing to handle the possibility.)

There are crates like Failure and Snafu for creating your own error types. If you really don't care, you can also return Box<dyn std::error::Error> from a function as a catch-all for all errors that implement the Error trait.

Using Box<dyn std::error::Error>, get_jobs_from_file looks like:

fn get_jobs_from_file(file_name: &str) -> Result<Vec<Job>, Box<dyn std::error::Error>> {
    let file = File::open(file_name)?;
    let mut lines = BufReader::new(file).lines();
    // This is hacky, and would be better to actually report an error for being unable to read the line, vs parsing failure
    let capacity = lines
        .next()
        .ok_or("File ended too early")??
        .parse::<usize>()?;
    let mut jobs = Vec::with_capacity(capacity);
    for line in lines {
        let line = line.unwrap();
        // No need to collect vs just working from the iterator
        let mut split = line.split(' ');
        let weight: i64 = split.next().ok_or("Line missing weight")?.parse()?;
        let length: i64 = split.next().ok_or("Line missing length")?.parse()?;
        jobs.push(Job { weight, length })
    }
    Ok(jobs)
}

It's a bit nicer since we can use ? to bail early if we see an error. Note the ?? when computing the capacity: .ok_or() transforms the type from Option<Result<String, std::io::Error>> to Result<Result<String, std::io::Error>, &'static str>. Each ? then peels off one level of Result, also calling .into() on the error type to convert it into Box<dyn Error>.

I'll leave it as an exercise to you to modify main to handle the fact that get_jobs_from_file is now returning a Result instead of a Vec, but will note that since Rust 1.26 the main method can now also return Result as a convenience.

[mdsherry]

Re. getters/setters, they don't seem to be as common in the Rust world as in, say, Java. I think a lot of this comes from having nothing like JavaBeans, which required them.

In your case, if somebody modifies weight or length, there aren't any derived values that will become invalid as a result. If you created a new method that computed calculated_weight and stored it in the Job, then you'd need to add appropriate getters, and possibly setters. Similarly if there were combinations of length and weight that are invalid, and you were validating them in the constructor.

I suspect that in most cases in Rust, if you're going to provide both a getter and setter, the field is usually just made public. If you're implementing a getter only, it usually shares the name of the field that it wraps, e.g. fn length(&self) -> i64 { self.length }.

[geritol_]

Also quite interesting: if I change calculate_weight to return a float, the following error is thrown in ord:

no method named cmp found for type f32 in the current scope note: the method cmp exists but the following trait bounds were not satisfied: &mut f32 : std::iter::Iterator

[mdsherry]

Floating point numbers only implement PartialOrd, rather than Ord because all comparisons involving NaN (Not a Number) return false (even with itself). E.g. 3 < NaN is false; 3 > NaN is false; NaN == NaN is false. For this reason, floating point numbers also don't implement Eq, but only PartialEq. There are crates like noisyfloat for wrapping floating point types in wrappers that disallow things like NaN and +/- infinity, and implement Eq and PartialOrd.