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.

Is there any way to simplify it further?

import java.util.*;

class Triangle {

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);
        int a = sc.nextInt();
        int b = sc.nextInt();
        int c = sc.nextInt();


        if(a==b && b==c)
            System.out.println("Equilateral");

        else if(a >= (b+c) || c >= (b+a) || b >= (a+c) )
            System.out.println("Not a triangle");

        else if ((a==b && b!=c ) || (a!=b && c==a) || (c==b && c!=a))
            System.out.println("Isosceles");

        else if(a!=b && b!=c && c!=a)
            System.out.println("Scalene");
    }
}
share|improve this question

3 Answers 3

Is there any way to simplify it further?

Sure, but what's more important: Create some useful methods. Your class is single shot only: read data, classify, output. This is a bad start, a method should do one thing only. So you need three methods.

You classify into 4 categories and this is a clear case for an enum:

enum TriangleType {
    EQUILATERAL, INVALID, ISOSCELES, SCALENE
}

This is the only interesting method:

static TriangleType classify(int a, int b, int c) {
    if (a <= 0 || b <= 0 || c <= 0) return INVALID; // added test
    if (a == b && b == c) return EQUILATERAL;
    if (a >= b+c || c >= b+a || b >= a+c) return INVALID;
    if (b==c || a==b || c==a)) return ISOSCELES;
    return SCALENE;
}

Note that I'm violating the "braces everywhere" rule. I can't help myself.

In case of ISOSCELES, there's no need for checks like a==b && b!=c as we already know that not all sites are equal.

In case of SCALENE, there's no need for any test, as we know that there are no two equals sides.


You're using a bit too many parentheses, but feel free to keep them if you feel you need them.

Be consequent with your spacing, there are rules for it, which help to make it more readable:

  • single space between "if" and "("
  • (but no space between a method name and "(")
  • no space after "("
  • no space before ")"

The addition is subject to overflow as Ryan noted, but let's ignore it for now. You should know about it and do a corresponding test if needed, but in general, overflow is impossible to check for everywhere.

Here it's trivial, just replace a >= b+c by a >= (long) b+c. The latter means a >= ((long) b) + c, which is equivalent to ((long) a) >= ((long) b) + ((long) c).

share|improve this answer
3  
Not sure if it is a problem, but one of the sums (i.e. a+b) may cause an integer overflow, which does not give INVALID. –  Ryan yesterday
    
@Ryan If the OP is using Java 8, the overflow can be handled using the new Math.addExact() method which would throw an ArithmeticException in the presence of overflows. –  Edwin Dalorzo 13 hours ago
    
Do you know about the performance of Math.addExact(int, int)? I cannot seem to find anything on it. For the lazy: docs.oracle.com/javase/8/docs/api/java/lang/… (regarding @EdwinDalorzo's comment) –  Ryan 13 hours ago
    
@Ryan Here's the link: Math.addExact. You can read the JDK 8 source code for performance. I am sure that'll not be an issue. You'll see implementation is quite simple and straightforward. You could even do it with JDK 7 by implementing the same logic. –  Edwin Dalorzo 13 hours ago
    
@Ryan I guess, the performance is fine in case the exception gets rarely thrown (just a predicted branch, costs 0 or 1 cycle on common HW as it can execute in parallel with other operations). If it throws, then it takes long, and if it throws often, then the branch gets hard to predict and performance suffers. I would not use it except when the exception is the wanted outcome (and occurs exceptionally only). If you want just a safer check, then see my update. –  maaartinus 13 hours ago

as an addition to all already mentioned points:

import java.util.*;

This is dumb!

The Java Util-Package contains a large amount of classes, interfaces and exceptions. With that single line, you make Eclipse / Netbeans "Do what I want Button" (tm) - also known as "completion suggestion" - load ALL THE CLASSES:

how not to do it

What exactly do you use of that? right, the Scanner. Aside from being hugely inconvenient, it clutters your namespace and can lead to naming conflicts.

Instead do this:

import java.util.Scanner;

Additional "nitpicks:"

  • Place braces around if and else blocks...
  • Make your variable names more speaking than a, b, c, even though it's hard with triangle-sides
  • Improve User Experience (UX). If someone executes this, what are they supposed to enter??
share|improve this answer
2  
I strongly doubt that all the classes get loaded! –  maaartinus yesterday
    
@maaartinus hmm... possibly. The IDE's "Do what I want button"(tm) actually loads all the **** you got there, though. Additionally I think that (while the classes may not actually be "loaded"), they are loaded into the runtime constant pool, which consumes memory.. –  Vogel612 yesterday
1  
I doubt the constant pool, too. The imports are a source only feature and the constant pool contains FQNs only. I guess every compiler emits only needed constants. –  maaartinus yesterday
    
@maaartinus isn't the wildcard import just a shorthand for specifying all FQNs at once? At least I understood it that way... How else would wildcard imports actually work without throwing LinkageErrors to infinity? –  Vogel612 yesterday
3  
AFAIK it's only syntactic sugar. Something the compiler uses (for replacing all simple names by FQNs) and forgets at the very beginning. If it sees java.util.* it resolves e.g., List against it (assuming no conflicts), but that's all. –  maaartinus yesterday

There's a lot of redundancy due to symmetry, since a, b, and c are interchangeable. Whenever you have a group of variables that should be treated the same way as each other, you should consider using an array instead.

Once you sort the sides by length, the problem becomes easier. Need to check for a negative length (which you didn't do)? Just check the smallest number. Need to detect an equilateral triangle? Just check the "shortest" and "longest" sides for equality.

In one of the comments, @Ryan pointed out that adding two sides could result in overflow. Therefore, subtracting is safer.

int[] s = new int[3];   // Lengths of the sides
try (Scanner sc = new Scanner(System.in)) {
    for (int i = 0; i < s.length; i++) {
        s[i] = sc.nextInt();
    }
    Arrays.sort(s);

    if (s[0] <= 0) {
        throw new IllegalArgumentException("Side with non-positive length");
    }

    System.out.println( (s[0] == s[2]) ? "Equilateral"
                      : (s[2] - s[1] >= s[0]) ? "Not a triangle" 
                      : (s[1] == s[0] || s[1] == s[2]) ? "Isosceles"
                      : "Scalene");
}
share|improve this answer
3  
TBH I find the approach OP is taking far more obvious and easy to understand.. Additionally IMO there must be a special place in hell for people who nest ternary statements, but eh. –  Vogel612 yesterday
1  
Why the downvote? Unless I'm getting really rusty with high-school trigonometry, sorting the sides first does simplify the comparison as shown in the code. I guess it boils down to trading any significant performance hits for Arrays.sort() for simpler if-comparisons? –  h.j.k. yesterday
    
@h.j.k. I downvoted for the loss in obviousness of what the if statements actually do, and how they internally work. –  Vogel612 20 hours ago
    
@Vogel612, after sorting: if the shortest and longest sides are the same, it's an equilateral triangle. If the longest side is greater than the sum of two other sides (en.wikipedia.org/wiki/Triangle_inequality), it's not a triangle. If any two sides are the same (after checking for equilateral triangle), it's an isosceles triangle. Otherwise, default to scalene triangle. –  h.j.k. 19 hours ago
    
@Vogel612 Ternary chaining works perfectly well in every language except PHP. I think that the usage above is almost English-like in readability. Shortest side equals longest side? Equilateral. Difference between the two longest sides exceeds the shortest side? Not a triangle. Two equal sides? Isosceles. Otherwise, scalene. –  user50399 19 hours ago

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.