# Feedback for checking if a triangle is equilateral

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).

7 Likes

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:

``````fn equilateral(dimensions: Triangle) -> bool {
dimensions.a > 0.0 && dimensions.a == dimensions.b && dimensions.a == dimensions.c
}
``````
1 Like

What would be the return type for `pub fn new` in `impl Triangle { ... }` for returning `None` and `Err` ?

`Option<Self>` and `Result<Self, SomeError>`, respectively.

3 Likes

Thanks @jwodder , @parasyte and @H2CO3 , I have the below based on your suggestions:

``````#[derive(Copy, Clone)]
struct Triangle {
a: f64,
b: f64,
c: f64,
}

impl Triangle {
pub fn new(a: f64, b: f64, c: f64) -> Option<Self> {
if a == 0.0 && b == 0.0 && c == 0.0 { // validate lengths
None
} else if a + b <= c && b + c <= a && a + c <= b { // validate sum of sides
None
} else {
Some(Triangle{a: a, b: b, c: c})
}
}
}

fn equilateral(dimensions: Triangle) -> bool {
dimensions.a == dimensions.b && dimensions.a == dimensions.c
}

#[cfg(test)]
mod tests {

use super::*;

#[test]
fn triangle_validity() {
let scalene_triangle = Triangle::new(7.0, 2.0, 5.0);
let right_angled_triangle = Triangle::new(13.0, 5.0, 12.0);
let isosceles_triangle = Triangle::new(6.0, 8.0, 6.0);
let equilateral_triangle = Triangle::new(10.0, 10.0, 10.0);

assert!(!equilateral(scalene_triangle));
assert!(!equilateral(isosceles_triangle));
assert!(!equilateral(right_angled_triangle));
assert!(equilateral(equilateral_triangle));
}
}
``````

However, for each assertion, the test fails with:

``````note: expected struct `triangle::Triangle`
found enum `Option<triangle::Triangle>`
``````

How can I solve this ?

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();
``````
3 Likes

Thank you so much for the detailed explanation !!

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).

2 Likes

Thanks !

Also consider making equilateral an inherent method of the struct Triangle:

``````impl Triangle {
fn equilateral(&self) -> bool {
self.a == self.b && self.a == self.c
}
}
``````

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.

6 Likes

As a more practical example one could name this:

``````#[test]
fn triangle_validity() {
let should_be = Triangle::new(0.3, 0.2 + 0.1, 0.3).unwrap();
assert!(equilateral(should_be));
}
``````

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.

2 Likes

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.

That's a great example !!

My tests currently look like:

``````#[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));
}
``````
1 Like

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`:

``````fn equilateral(dimensions: &Triangle) -> bool {
dimensions.a == dimensions.b && dimensions.a == dimensions.c
}
``````

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.

1 Like

Comparing floats in equality is a bad practice, for example:

``````let equilateral_triangle = Triangle::new(0.15 + 0.15, 0.1 + 0.2, 0.3).unwrap();
``````

fails

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:

1. `cargo fmt` to format it nicely.
2. `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.

3 Likes

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.