I'm learning Rust and have covered topics like numbers, booleans, and control flow. I recently began with the structs topic and have a program which checks if a triangle is equilateral or not:
#[derive(Copy, Clone)]
struct Triangle {
a: f64,
b: f64,
c: f64,
}
/// Returns false if any side of a triangle is <= 0
fn validate_lengths(dimensions: Triangle) -> bool {
if dimensions.a <= 0.0 {
false
} else if dimensions.b <= 0.0 {
false
} else if dimensions.c <= 0.0 {
false
} else {
true
}
}
/// Returns false if the sum of any two sides < third side
fn validate_side_sums(dimensions: Triangle) -> bool {
if dimensions.a + dimensions.b < dimensions.c {
false
} else if dimensions.b + dimensions.c < dimensions.a {
false
} else if dimensions.a + dimensions.c < dimensions.b {
false
} else {
true
}
}
fn equilateral(dimensions: Triangle) -> bool {
let valid_triangle = validate_lengths(dimensions) && validate_side_sums(dimensions);
if valid_triangle == true {
let mut equilateral: bool = false;
// implies side a != 0 because of valid_triangle
if dimensions.a == dimensions.b && dimensions.a == dimensions.c {
equilateral = true;
true
} else {
false
}
} else {
false
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn triangle_validity() {
let scalene_triangle = Triangle {
a: 7.0,
b: 2.0,
c: 5.0,
};
let right_angled_triangle = Triangle {
a: 13.0,
b: 5.0,
c: 12.0,
};
let isosceles_triangle = Triangle {
a: 6.0,
b: 8.0,
c: 6.0,
};
let equilateral_triangle = Triangle {
a: 10.0,
b: 10.0,
c: 10.0,
};
assert_eq!(equilateral(scalene_triangle), false);
assert_eq!(equilateral(isosceles_triangle), false);
assert_eq!(equilateral(right_angled_triangle), false);
assert_eq!(equilateral(equilateral_triangle), true);
}
}
I will expand the program to make checks for isosceles and scalene triangles, and the valid_triangle variable check in equilateral() function will be repeated.
Please provide feedback to improve the above program, specially on beginner topics like control flow and impl block (if its a good idea in my case).
First: Instead of making validate_lengths() and validate_side_sums() functions, you should make their code part of a Triangle::new(a, b, c) constructor that is always used to validate potential new triangles, and if the triangle doesn't pass validation, new() should return either None or Err(something) (your choice). This is known as "Parse, don't validate", and it lets you be sure that every Triangle object is valid without having to validate each object every time you want to use it.
Secondly, regarding correctness, validate_side_sums() should return false if any of the comparisons are <=, not just <, as when the sum of two sides equals the third, the "triangle" will be a straight line, which is not good.
Also, yes, as the functions you've written are conceptually tightly associated with the Triangle type, they should be methods rather than functions by being placed inside an impl Triangle { ... } block.
There's no reason to have an equilateral variable inside the equilateral() function, as you're not doing anything with the value.
assert_eq!(equilateral(scalene_triangle), false); can be written in a shorter form as assert!(!equilateral(scalene_triangle)); (note the ! inside the outermost parentheses, as you're asserting that equilateral(scalene_triangle) is not true).
Algorithmically, the code is more complex than it needs to be. The definition of an equilateral triangle is "all sides have the same length". If we ignore degenerate cases where a side is <= 0.0, we get this solution:
In let scalene_triangle = Triangle::new(7.0, 2.0, 5.0); and the lines following, Triangle::new(...) is returning an Option<Triangle> instead of just a Triangle. The simplest way to convert an Option to the inner value is to call Option::unwrap(), which will make your program panic if the triangle didn't pass validation — but you're expecting your triangles to pass validation, so if that happens, your test will fail, telling you that your code is broken somewhere, and so your test will have done its job.
Anyway, just add .unwrap() like this:
let scalene_triangle = Triangle::new(7.0, 2.0, 5.0).unwrap();
Your checks are wrong, they should use || instead of &&. A triangle is already degenerate if one pair of sides is shorter than the other (or negative).
It should be noted that floating point types (f32, f64, etc) are only approximations of actual numbers, and test for equality are therefore not meaningful most of the time, i.e. two values that should be equal in theory might not turn out to be equal. Typically, a test is done whether values are equal within an acceptable range. The best solution depends on why you want to check whether a triangle is equilateral.
You will be able to see that the test fails, even though the numbers are clearly equivalent numerically - this is exactly because of the precision issues @curoli mentioned.
The tests that I have (and will have when I expand this to include isosceles and scalene) only perform boolean checks on triangle type. My main goal with this exercise is to understand structs.
I have tested floats with other problems by rounding up or flooring to two decimal places - they are simple approximations and nothing fancy.
#[test]
fn invalid_triangles() {
let invalid_triangle_one = Triangle::new(2.0, 0.0, 2.0);
let invalid_triangle_two = Triangle::new(2.0, 1.0, 3.0);
assert!(
matches!(invalid_triangle_one, None),
"one side of the triangle is of zero length"
);
assert!(
matches!(invalid_triangle_two, None),
"sum of two sides not greater than third"
);
}
#[test]
fn valid_equilateral_triangle() {
let scalene_triangle = Triangle::new(7.0, 2.0, 6.0).unwrap();
let right_angled_triangle = Triangle::new(13.0, 5.0, 12.0).unwrap();
let isosceles_triangle = Triangle::new(6.0, 8.0, 6.0).unwrap();
let equilateral_triangle = Triangle::new(10.0, 10.0, 10.0).unwrap();
assert!(!equilateral(scalene_triangle));
assert!(!equilateral(isosceles_triangle));
assert!(!equilateral(right_angled_triangle));
assert!(equilateral(equilateral_triangle));
}
It might be a minor point for this particular exercise, but in future work I might consider changing the equilateral function to take in a reference to Triangle, i.e. &Triangle:
In this case your version works because you derive Copy on the Triangle, but I don't think there's any reason for the function to take ownership of the triangle.
Another good practice is using cargo clippy to lint the project. I copy-pasted much of the code from this thread into a project and ran two commands over it:
cargo fmt to format it nicely.
cargo clippy to check for lint.
And these can be automated and integrated with your IDE with rust-analyzer for much better visibility.
If you also add #![deny(clippy::pedantic)] to the top, you get even more stringent checks, including the issue @curoli pointed out. The help link for it is float_cmp.
Most people dislike this lint level because it has many false positives, but it does catch bugs like this one.