Function for determining triangle type

Question

A while back I was asked to write some sample code for a job that I was applying for. They wanted a class that had a function that would accept three lengths and return the appropriate type of triangle (Scalene, Isosceles or Equilateral) based on that. They also wanted unit tests.

After sending this I never heard back, so I'm wondering if anyone would have any suggestions for a better way to implement this.

using NUnit.Framework;

namespace Triangle
{
    /****
     *  This test is divided into two halves.
     *  1. Implement the GetTriangleType method so that it returns the appropriate
     *     enum value for different inputs.
     *  2. Write a complete series of passing unit tests for the GetTriangleType method
     *     under various input conditions. In this example we're using NUnit, but
     *     you can use whatever testing framework you prefer.
     *     
     *  When finished, send back your solution (your version of TriangleTester.cs
     *  will suffice) including any explanatory comments you feel are needed.  
     */

    public enum TriangleType
    {
        Scalene = 1, // no two sides are the same length
        Isosceles = 2, // two sides are the same length and one differs
        Equilateral = 3, // all sides are the same length
        Error = 4 // inputs can't produce a triangle
    }

    public class TriangleTester
    {
        /// <summary>
        /// Given the side lengths a, b, and c, determine and return
        /// what type of triangle the lengths describe, or whether
        /// the input is invalid
        /// </summary>
        /// <param name="a">length of side a</param>
        /// <param name="b">length of side b</param>
        /// <param name="c">length of side c</param>
        /// <returns>The triangle type based on the number of matching sides passed in.</returns>
        public static TriangleType GetTriangleType(int a, int b, int c)
        {
            //Placing items in an array for processing 
            int[] values = new int[3] {a, b, c};

            // keeping this as the first check in case someone passes invalid parameters that could also be a triangle type. 
            //Example: -2,-2,-2 could return Equilateral instead of Error without this check.  
            //We also have a catch all at the end that returns Error if no other condition was met.
            if (a <= 0 || b <= 0 || c <= 0) 
            {
                return TriangleType.Error;
            }
            else if (values.Distinct().Count() == 1) //There is only one distinct value in the set, therefore all sides are of equal length
            {
                return TriangleType.Equilateral;
            }
            else if (values.Distinct().Count() == 2) //There are only two distinct values in the set, therefore two sides are equal and one is not
            {
                return TriangleType.Isosceles;
            }
            else if (values.Distinct().Count() == 3) // There are three distinct values in the set, therefore no sides are equal
            {
                return TriangleType.Scalene;
            }
            else
            {
                return TriangleType.Error;
            }
        }
    }

    [TestFixture]
    public class TriangleTesterTests
    {
        [Test]
        public void Test_GetTriangleType()
        {
            Assert.AreEqual(TriangleType.Equilateral, 
                            TriangleTester.GetTriangleType(4, 4, 4), 
                            "GetTriangleType(4, 4, 4) did not return Equilateral");

            Assert.AreEqual(TriangleType.Isosceles, 
                            TriangleTester.GetTriangleType(4, 4, 3),
                            "GetTriangleType(4, 4, 3) did not return Isosceles");

            Assert.AreEqual(TriangleType.Scalene, 
                            TriangleTester.GetTriangleType(4, 3, 2),
                            "GetTriangleType(4, 3, 2) did not return Scalene");

            Assert.AreEqual(TriangleType.Error, 
                            TriangleTester.GetTriangleType(-4, 4, 4),
                            "GetTriangleType(-4, 4, 4) did not return Error");

            Assert.AreEqual(TriangleType.Error,
                            TriangleTester.GetTriangleType(4, -4, 4),
                            "GetTriangleType(4, -4, 4) did not return Error");

            Assert.AreEqual(TriangleType.Error,
                            TriangleTester.GetTriangleType(4, 4, -4),
                            "GetTriangleType(4, 4, -4) did not return Error");
        }
    }

}

Answer 1

I don't code C#, so just some generic notes:

1. You can omit the else keyword if you return immediately:

   if (a <= 0 || b <= 0 || c <= 0) 
   {
       return TriangleType.Error;
   }
   if (values.Distinct().Count() == 1) //There is only one distinct value in the set, therefore all sides are of equal length
   {
       return TriangleType.Equilateral;
   }
   ...
   

2. Some comments are just says what's in the code. I'd remove them, they're just noise.

3. The code doesn't check that the the sum of the lengths of any two sides of the triangle have to be greater than the length of the third side. (a + b > c)

4. About the specification: in case of an error you might want to throw an exception with a detailed error message instead of the Error enum which tells nothing about the cause of the error to the clients.

5. Just a link for @Jeff's point: Too many assert in one test is a bad smell. If the first AreEqual throws an exception you won't know anything about the results of the other assert calls which could be important because they could help debugging and defect localization.

Answer 2

I don't see anything wrong with your standards.

I do see some design issues though.

The biggest one is your unit test. I count 6 distinct unit tests, not 1. Each one of those asserts should be in its own test with a method name describing the test. Assuming you are using NUnit. I would also use Assert.That, but I think that is just personal preference:

[Test]
public void ThreeEqalValuesShouldReturnEquilateral()
{
     Assert.That(TriangleTester.GetTriangleType(4, 4, 4), Is.EqualTo(TriangleType.Equilateral);
}

Again, if using Nunit, another option would be to use the TestCase attribute:

[Test]
[TestCase(4, 4, 4, TriangleType.Equilateral)]
[TestCase(4, 4, 3, TriangleType.Isosceles)]
// ...
public void CheckDifferentTriangleTypes(int a, int b, int c, TriangleType expected )
{
     Assert.That(TriangleTester.GetTriangleType(a, b, c), Is.EqualTo(expected);
}

That being the biggest issue, but I think you could of done something with that monster if/else statement in your main program. I also think you over commented it. What I mean is that your comments are explaining the logic that most people would be able to read.

An easy way to fix this is to create functions for each check. Then you don't need to comment. Also, because each if / else has a return in it, I you can lose the elses. I would also move the assignment of the array until after the first check.

if (a <= 0 || b <= 0 || c <= 0) 
{
    return TriangleType.Error;
}

int[] values = new int[3] {a, b, c};

if (AllThreeSidesAreEqual(values)) 
{
    return TriangleType.Equilateral;
}

if (TwoSidesAreEqual(values))
{
    return TriangleType.Isosceles;
}

if (NoSidesAreEqual(values))
{
    return TriangleType.Scalene;
}

return TriangleType.Error;

where

private static bool AllThreeSidesAreEqual(int[] values)
{
     return values.Distinct().Count() == 1;
}

// Build other methods

Try fixing these and look at your code again. Hopefully you will see a difference in the two ways of doing it.

Good luck.

Answer 3

Some style suggestions:

• Move the argument bounds checking to the top of the method. No sense allocating the array (even if it's small) if the values aren't valid in the first place. This is also a different check than the one you are doing to test the triangle so it doesn't really belong in that if-else chain.

A different look (comments snipped, most are self explanatory):

    public static TriangleType GetTriangleType(int a, int b, int c)
    {
        // There should also be a side length check
        if (a <= 0 || b <= 0 || c <= 0) 
        {
            return TriangleType.Error;
        }

        if (a == b && a == c) // These could also be their own methods
        {
            return TriangleType.Equilateral;
        }
        else if (a == b || a == c || b == c)
        {
            return TriangleType.Isosceles;
        }
        else
        {
            return TriangleType.Scalene;
        }
    }

Edit: Maybe a static utility method isn't the best in this case either (rather a Triangle class). Your best bet is if you still have a contact for whoever you submitted this code to, perhaps you could send a follow-up asking for feedback.

Answer 4

Maybe I'm missing the obvious but... shouldn't you first check if the values actually form a valid triangle?

For instance, entering lengths 10, 10, 100000 wouldn't make it a valid isosceles triangle.

... just saying.

For valid triangle lengths you can check this stack overflow question.

Answer 5

OK, I don't do C#, (I come from VB) I'll have the wrong syntax in some instances, but I think you'll understand what I'm driving at.

My idea would be to have an enum who's numerical value is the number of similar sides

Enum TriangleType as integer
    Error = -1     //cannot make a triangle
    Scalene = 0    //no sides equal
    Isosceles = 2   //2 sides equal
    Equilateral = 3  //3 sides equal
End Enum

Then you have the calling functions

public sub GetInputs as integer()
//whatever code you need to get inputs here

int InputArray[] = {a,b,c} //create an array with the values

 //Calculate results and write to the screen
 int ThisTriangleType = GetTriangleType(InputArray)
 Console.Write (ThisTriangleType.Tostring)
 Console.Write(" has ")
 Console.Write(ThisTriangleType)
 Console.Writeline(" equal sides")

end sub


public function GetTriangleType(int IntegerArray*) as TriangleType
   IntegerArray.sort(numerical ascending)
   if IntegerArray(0) <=0 then
       //smallest (thus all values) must be greater than zero
       Return TriangleType.Error 
   end if

   if IntegerArray(0) == IntegerArray(1) and IntegerArray(0)== IntegerArray(2)
        //We have an Equilateral Triangle
        Return TriangleType.Equilateral
   end if

   if IntegerArray(0) == IntegerArray(1) then
        //the two short sides are equal, 
        //Long side must be less than their sum
        if IntegerArray(0) + IntegerArray(1) > IntegerArray(2)
             Return TriangleType.Error
        End if
        //We have a valid Isosceles triangle with <60* angle
        Return TriangleType.Isosceles
   End if

   if IntegerArray(1) == IntegerArray(2) then
       //the two long sides are equal
       //automatically valid Isoscelese with a >60* angle
       Return TriangleType.Isosceles
   End if 

   if IntegerArray(0) + IntegerArray(1) > IntegerArray(2) then
       //We've tested for all conditions with 2 or more equal lengths
       //We have a valid Scalene since the longest is less 
       //than the sum of the 2 shortest
       Return TriangleType.Scalene
   end if
   //We have an invalid triangle
   Return TriangleType.Error
end function

I'm sorry for the mixing of VB and C..