# Function for determining triangle type

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.
*
*  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");
}
}

}


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.

• Good point on the a+b>c error. I didn't even think of that until just now. – Abe Miessler Nov 5 '12 at 18:38

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.

• In addition, they might be looking for the class to implement an interface and not have a static method so that the method can be mocked out of any calling methods. – Gene S Nov 5 '12 at 18:17
• That crossed my mind too. – Jeff Vanzella Nov 5 '12 at 18:18
• Good info on the unit tests, thanks. I have very little experience using them so I'm not surprised they are a little screwy. – Abe Miessler Nov 5 '12 at 18:39

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.

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.

• Yes, @palacsint pointed that out as well. – Abe Miessler Nov 6 '12 at 18:03

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

• As a side note, while this has some good information, you should either (re)write equivalent code in the language you know/pseudocode, or stick to a pure text description, if you don't know the language. – Fund Monica's Lawsuit Jan 16 '16 at 1:54
• @QPaysTaxes I think it's fine. Any C# developer should be able to read VB.Net as "pseudo code", even if they're not familiar with VB. – RubberDuck Jan 16 '16 at 12:31
• @RubberDuck Please note that I said "write it in a language you know" as an option. – Fund Monica's Lawsuit Jan 16 '16 at 22:00