Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The logic is simple. But I was wondering if anyone could point me into better rewriting the conditional logic? Also, the only error conditions i can think of are the sides should not be equal to or less than zero. Is there any other error conditions that seem to occur to you?

public class TriangleType {

    static Triangle getType(int a, int b, int c)
    {
        if(a<=0||b<=0||c<=0)
            throw new IllegalArgumentException("Length of sides cannot be equal to or less than zero");

        if(a==b&&b==c&&c==a)
            return Triangle.EQUILATERAL;
        else if((a==b)||(b==c)||(c==a))
            return Triangle.ISOSCELES;
        else if(a!=b&&b!=c&&c!=a)
            return Triangle.SCALENE;
        else
            return Triangle.ERROR;
    }


    public static void main(String[] args)
    {
        System.out.println(TriangleType.getType(13, 13, 0));

    }
}

enum Triangle
{
ISOSCELES(0),
EQUILATERAL(1),
SCALENE(2),
ERROR(3);

private int n;
Triangle(int n)
{this.n = n;}
}
share|improve this question
add comment

3 Answers

You could encapsulate the conditions in a PotentialTriangle class. Note that the fields are of type long to avoid overflow when calculating whether triangle inequality is violated.

public class PotentialTriangle {
    private final long a, b, c;

    public PotentialTriangle(int sideA, int sideB, int sideC) {
        a = sideA;
        b = sideB;
        c = sideC;
    }

    public boolean isAnySideTooShort() {
        return a <= 0 || b <= 0 || c <= 0;
    }

    public boolean violatesTriangleInequality() {
        return a > b + c || b > a + c || c > a + b;
    }

    public boolean areSidesEqual() {
        return a == b && b == c;
    }

    public boolean areAtLeastTwoSidesEqual() {
        return a == b || b == c || c == a;
    }
}

This would vastly simplify your enum selection code, which I would put directly into the enum:

public enum TriangleType {
    ISOSCELES,
    EQUILATERAL,
    SCALENE;

    public static TriangleType ofPotentialTriangle(PotentialTriangle triangle) {
        throwIf(triangle.isAnySideTooShort(), 
                "Length of sides cannot be equal to or less than zero");
        throwIf(triangle.violatesTriangleInequality(), 
                "Sum of any two sides must be larger than the remaining side");

        if (triangle.areSidesEqual()) {
            return EQUILATERAL;
        }
        if (triangle.areAtLeastTwoSidesEqual()) {
            return ISOSCELES;
        }
        return SCALENE;
    }

    private static void throwIf(boolean condition, String message) {
        if (condition) {
            throw new IllegalArgumentException(message);
        }
    }
}
share|improve this answer
add comment

The code seems to work...

For enhancements:

if (a == b && b == c && c== a)

is redundant. Shorter is

if (a == b && b== c)

because in this case a == c holds always true.

Also you should test the following

if (a + b < c || a + c < b || b + c < a)

because in this case it is no triangle.

Else I personally cannot see how to really improve the conditions.

share|improve this answer
    
Actually, if a + b > c etc. then it is a valid triangle; if a + b < c etc. then the triangle is invalid. –  codesparkle Nov 11 '12 at 17:53
    
My fault. I meant c > a + b. Long time ago that I had math in school. Corrected answer. –  Uwe Plonus Nov 11 '12 at 18:52
    
One post below points out only the longest edge cannot be less than sum of the other two. Do we test only that or sum of any two must not be less than the third ? –  Phoenix Nov 11 '12 at 21:32
    
I don't understand your question. The longest edge cannot be longer than the sum of the other two therefore we have to check all edges. The code in the next answer does the same as my code, it is only structured in another way. –  Uwe Plonus Nov 11 '12 at 22:26
    
I meant to say you are checking for sum of any two sides not greater than 3rd (for all the sides) but the code below checks only for the max side is less than sum of the other two sides. –  Phoenix Nov 12 '12 at 0:40
show 1 more comment

You've missed an error case called the "triangle inequality": the longest edge can't be longer than the sum of the other two. (If it's the same length then you probably want to consider that an error too because it reduces to a line segment). To test this robustly requires taking into account overflow.

It's a bit odd that you throw an Exception for some errors but return an error value for others. Since you're writing Java rather than C, favour throwing exceptions for exceptional cases.

    static Triangle getType(int a, int b, int c)
    {
        if(a<=0||b<=0||c<=0)
            throw new IllegalArgumentException("Length of sides cannot be equal to or less than zero");

        int max = Math.max(Math.max(a, b), c); // Or use :? if you prefer
        if (max == a)
            checkTriangleInequality(a, b, c);
        else if (max == b)
            checkTriangleInequality(b, a, c);
        else checkTriangleInequality(c, a, b);

        if(a==b&&b==c)
            return Triangle.EQUILATERAL;
        else if((a==b)||(b==c)||(c==a))
            return Triangle.ISOSCELES;
        else return Triangle.SCALENE;
    }

    private static void checkTriangleInequality(int max, int x, int y)
    {
        // Assume that we've already checked all three are > 0.
        // Therefore if x + y < 0 the sum overflowed and is greater than max.
        if (x + y > 0 && x + y <= max)
            throw new IllegalArgumentException("Triangle inequality violated");
    }
share|improve this answer
    
Thanks for the input. How would you deal with overflow ? –  Phoenix Nov 11 '12 at 18:18
    
@Phoenix, see the comment in checkTriangleInequality. –  Peter Taylor Nov 11 '12 at 23:07
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.