Implement Triangle

[IanWhitney]

Test suite mostly follows the standard. I've started off with a couple of Result-checking tests, just to establish that we expect a Result back.

The floating-point tests are there, but commented out. Currently support for them is unimplemented. Still trying to decide if they should be implemented.

The approach I'm taking here is the Builder pattern, more-or-less. The Triangle struct does some boundary checking on the sides, but then return a Box containing one of the three concrete TriangleType implementations.

This API approach is, um, probably not the easiest. I know it's tricky because I had to ask Steve Klabnik if it was even possible

On the plus side, it introduces Boxes, which currently no problem uses. And it shows how you can do inheritance-like things in Rust.

That said, if everyone thinks it's a bad idea we can revisit.

Todo:

• Decide the API is sane
• Decide if we're going to do floating points.
• Populate the HINTS.md file
• Place the problem in config.json
• Replace my initial example with one that is less crazy

[petertseng]

consider an if let here to avoid an unwrap below.

[petertseng]

This API approach is, um, probably not the easiest. I know it's tricky because I had to ask Steve Klabnik if it was even possible

On the plus side, it introduces Boxes, which currently no problem uses. And it shows how you can do inheritance-like things in Rust.

Well, this is just speaking about the example. The API imposed by the test does not seem to require boxes at all (unless I miss something). It's very possible that the student will simply have pub fn build(sides: [u16; 3]) -> Result<Triangle, ()>, right?

[petertseng]

I'm trying to reason through the advantages and disadvantages of having the api be is_equilateral, is_scalene, is_isosceles rather than simply kind whose result is an enum.

As written:

• We won't admit any trivial implementations like triangle: Ensure no pair of triangle types are equal go#208
  • if you say that's not possible because it's an enum, again that's making assumptions from the example. If the tests only expect that there's something called Equilateral and Isosceles, the student doesn't have to make those two things variants of a enum.
  • It is good that we don't admit the trivial implementations since we don't have to add a separate test that no pair of the values are equal.
• If the interface involved an enum, it would probably have to derive PartialEq (but that's not hard for the student, the enum can just derive(PartialEq))
• With this API, the student has freedom to choose how to represent the triangle types internally. It can be an enum, or it can be different types of triangles as the example solution has. We like giving student freedom.
• I think the only disadvantage I see is that this API doesn't make clear that only one of the three functions can be true. You could imagine someone writing an implementation where two or three of the functions return true. It would be wrong and fail the tests, of course, but now you have to rely on the test instead of the type system.

Since we are trying to decide whether the API is sane, please add any thoughts you can think of. I'll sleep on what I've got for now.

[petertseng]

Decide if we're going to do floating points.

The logic is all the same, so the only difference is whether the type becomes generic, right? It seems like this may depends on where we decide to place this problem. Any other considerations?

I can be convinced either way.

[IanWhitney]

Yes, with this API a student could do everything in the Triangle struct. No need for enums/boxes/etc. Something like:

fn build(sides: [u16;3]) -> Result<Triangle, ()> {
  let equilateral_test = //...;
  let isoscoles_test = //...;
  let scalene_test = //...;

  Ok(Triangle { equilateral: equilateral_test, //... });

(or, just create a Triangle struct with sides and have the is_ functions do the tests. Either way.)

I think this implementation is lacking. Maybe I'm wrong. There's no test in the current test suite that demonstrates any problems with this implementation.

If we wanted to force an implementation, we could write some test functions that require specific triangle types. Say a function that checks for Golden Triangleness and that only accepts Isosceles.

If we did come up with tests like this, I would prefer to do one of two things:

1. Clearly mark the tests as optional
2. Put the tests in a 2nd exercise, meant to show the effect of changing requirements on existing code. Kind of like my idea of a Knight Attack exercise following Queen Attack

[IanWhitney]

As for a Triangle enum, it would work. I tried that approach for a bit but changed tack. Partly because I'm just not a fan of Enums, and partly because I think they are a bad fit for the concept of Triangles.

For something that has a clear and distinct set of options enums can be great. But there are other ways of classifying triangles that makes Enum a bad choice.

Say you went with this enum:

enum Triangle {
  Equilateral,
  Isosceles,
  Scalene
}

And then you want to write is_obtuse. You can't make it a new variant, because it cuts across two of your existing variants. You can't make it a function in your variants because you can't write functions on enum variants. You have to put your is_obtuse() function up in Triangle. And so on with is_oblique and is_acute etc. Until all your behavior is in Triangle...so why not just use a plain-old Triangle struct to begin with?

[IanWhitney]

The logic is all the same, so the only difference is whether the type becomes generic, right? It seems like this may depends on where we decide to place this problem. Any other considerations?

The main hurdle for me is making the code generic over numeric types. Particularly the valid_sides function. This could just be a failing of my Rust knowledge. I think this is something the num crate helps with, but I need to play with that further.

[petertseng]

Yes, with this API a student could do everything in the Triangle struct. No need for enums/boxes/etc.

This also leads us to the question of: If we write a stub file (I notice none currently is), what is the signature we put in it?

I think this implementation is lacking. Maybe I'm wrong. There's no test in the current test suite that demonstrates any problems with this implementation.

What woud you say is lacking? But there are some implementation details that probably cannot be tested. (I'm thinking of the sieve problem, where you can't really test that the implementation uses the sieve algorithm, only that it gives you a list of primes)

For something that has a clear and distinct set of options enums can be great. But there are other ways of classifying triangles that makes Enum a bad choice.

Equilateral, Isosceles, and Scalene are the only options for the relationship between the sides, so to me this does tell me that enum is a great fit. And I would have another enum for right/obtuse/acute telling the relationship between the angles, since again a triangle can only be exactly one of these. So enum is again a great fit for that.

You can't make it a function in your variants because you can't write functions on enum variants

You can't write a function on values, but you can write a function on types.

enum Number {
    Zero,
    Succ(Box<Number>),
}

impl Number {
    fn is_zero(&self) -> bool {
        match *self {
            Number::Zero => true,
            _ => false,
        }
    }
}

fn main() {
    println!("{}", Number::Zero.is_zero());
    println!("{}", Number::Succ(Box::new(Number::Zero)).is_zero());
}

[petertseng]

However, despite my assertion that enums are a great fit for isosceles vs equilateral vs scalene, that says nothing about whether the tests need to require enums (they don't, by the way). So effectively I'm discussing how I would implement it, rather than how it's to be tested, and how I would implement it is less important here (that's for discussion on the site when I submit an implementation, not for this PR)

There is no need to change what's being tested right now, I don't think. I can be convinced otherwise, of course.

[petertseng]

The main hurdle for me is making the code generic over numeric types. Particularly the valid_sides function.

I tried. Indeed, it seems difficult. Attempting without num, you can say <T: PartialOrd + Add> but you also need a zero for the type you're dealing with. And also you have to constrain that the output of the add is comparable with another member of its type, because otherwise:

src/lib.rs:6:15: 6:34 error: binary operation `>=` cannot be applied to type `<T as std::ops::Add>::Output` [E0369]
src/lib.rs:6             ( sides[0] + sides[1] >= sides[2] )

Okay, what about...

    fn valid_sides<T: PartialOrd + std::ops::Add>(sides: [T; 3]) -> bool
        where <T as std::ops::Add>::Output: PartialOrd<T> {
            // omitted for now
    }

Ah, not quite.

src/lib.rs:7:15: 7:23 error: cannot move out of type `[T; 3]`, a non-copy fixed-size array [E0508]
src/lib.rs:7             ( sides[0] + sides[1] >= sides[2] )
                           ^~~~~~~~

Okay, maybe it works if I make it copy...

    fn valid_sides<T: Copy + PartialOrd + std::ops::Add>(sides: [T; 3]) -> bool
        where <T as std::ops::Add>::Output: PartialOrd<T> {
            ( sides[0] + sides[1] >= sides[2] )
            && ( sides[1] + sides[2] >= sides[0] )
            && ( sides[2] + sides[0] >= sides[1] )
    }

This works but notice I had to omit the zero check because I can't get a zero of T.

[petertseng]

Okay, now this type signature's just getting ridiculous.

    fn valid_sides<T: Copy + PartialOrd + std::ops::Add + std::ops::Sub>(sides: [T; 3]) -> bool
        where <T as std::ops::Add>::Output: PartialOrd<T>, <T as std::ops::Sub>::Output: PartialOrd<T> {
            let zero = sides[0] - sides[0];
            ( sides.iter().all(|&s| zero < s) )
            && ( sides[0] + sides[1] >= sides[2] )
            && ( sides[1] + sides[2] >= sides[0] )
            && ( sides[2] + sides[0] >= sides[1] )
    }

But it works.

#[cfg(test)]
mod tests {
    #[test]
    fn valid_uint() {
        let sides = [4, 3, 2]; 
        assert!(super::Triangle::valid_sides(sides));
    }

    #[test]
    fn uint_zero() {
        let sides = [0, 3, 2];
        assert!(!super::Triangle::valid_sides(sides));
    }

    #[test]
    fn nope_uint() {
        let sides = [7, 3, 2];
        assert!(!super::Triangle::valid_sides(sides));
    }

    #[test]
    fn valid_floats() {
        let sides = [0.4, 0.3, 0.2];
        assert!(super::Triangle::valid_sides(sides));
    }
}

I imagine that num can make this easier by giving us a zero for the type instead of having to synthesize one.

I'm not sure it's worth adding to the Cargo.toml that everyone will get though?

[IanWhitney]

And now I understand why the num crate exists. That gets pretty crazy.

I think if we include floats, we should include num. Since that's how real-world Rust programs would deal with this problem.

[petertseng]

Yeah makes sense. What was that problem we didn't end up adding the num crate to?

... ah, it was space age, because num wasn't essential to the tests. The difference here is that if we add float triangles then num will be sort of essential (well, OK, you could imagine an inelegant solution that just copies code between the <int> and <float> instances of valid_sides, but assuming that at least one student wants to avoid that!)

Think it's useful enough to introduce to students? My opinion has to be taken with a grain of salt since I'm pretty unknowledgeable about num. A note explaining to them what the purpose of num will probably be helpful.

[IanWhitney]

I'll try using num to allow the code to be generic over Floats & Ints. See how it goes.

[IanWhitney]

Oh, fun, all of my Triangle tests rely on Ord. And floats don't have Ord

[IanWhitney]

To do:

• Address @petertseng's style suggestions
• Place the problem in config.json
• Update problems.md

[petertseng]

I don't suppose it makes much of a difference, but what if maybe the float-compatible example was put into example_float.rs?

Or what if it were just made the default (and only) example?

[IanWhitney]

If I make it the only example, then I have to include num in Cargo.toml, which seems unnecessary. I'll go with making it a 2nd file.

[IanWhitney]

Thoughts on placement & topics covered? My example is not the way most students will go, I suspect. This works, right?

struct Triangle {
  sides: [u16;3]
}

impl Triangle {
  pub fn is_equilateral(&self) -> bool {
    //test sides for equality
  }

  //and so on
}

So if most students will go with an implementation like that, then we should place the problem around rna-transcription, I figure.

[petertseng]

I imagine a single struct will be a common approach.

I am having a hard time to think about topic. Math seems certain. Depending on implementation, enums or traits are potential topics. Basically what' already been said in HINTS.

I'm feeling indecisive about where to place, since I have a large range that I would have found acceptable: anywhere after leap, but before grade-school. Since our suggestions intersect and yours is smaller, take your pick?

Before I forget, another test was added in exercism/problem-specifications#364 after work on this was started.

[IanWhitney]

Since nothing in the tests suggests my weird-o implementation, I also plan on replacing the example code with a solution closer to what we expect students to know at this point in the track.

[IanWhitney]

Examples now simplified. I think we're wrapping this one up. Let me know.

[petertseng]

I think for all four of scalene tests you spelled it as traingle

[petertseng]

Another last thought I had: build vs new? In your mind does the choice of build imply a different semantics than new? If yes, keep build. Otherwise, it seems new would be more conventional

[IanWhitney]

I thought it was idiomatic for new to always return Self. If that's not true, I'm ok with new.

[petertseng]

I thought it was idiomatic for new to always return Self.

I reviewed a few examples and find this to be true for all examples I saw, with no counterexamples. I could maybe search the Rust standard libs, but it would take a while to sift through them all looking for any counterexample. Probably can keep it as is then.

[IanWhitney]

Rebased down to a single commit.

[IanWhitney]

Future reference, @petertseng : Generic over Float & Int without num

http://exercism.io/submissions/7137bcfcb706471ab10613b9b61c5977