4
\$\begingroup\$

This is my first program and I wanted to know if there was any way to improve the design or cut back on unnecessary coding.

import java.util.Scanner;


public class Main {

    public static void main(String args[]) {

        int side1; int side2; int side3;
        int side1s; int side2s; int side3s; 
        Boolean Triangle;
        String TriangleType;
        String Q1 = "What is the first side of the Triangle?";
        String Q2 = "What is the second side of the Triangle?";
        String Q3 = "What is the third side of the Triangle?";

        Scanner input = new Scanner(System.in);
        System.out.println(Q1);
        side1 = input.nextInt();

        Scanner input2 = new Scanner(System.in);
        System.out.println(Q2);
        side2 = input2.nextInt();

        Scanner input3 = new Scanner(System.in);
        System.out.println(Q3);
        side3 = input3.nextInt();

        if(side1 + side2 > side3) {
            Triangle = true;
            System.out.println("The triangle is true!");

        } else {
            Triangle = false;
            System.out.println("The Triangle is False!");
            return;

        }

        if(Triangle == true) {
            side1s = side1 * side1;
            side2s = side2 * side2;
            side3s = side3 * side3;

            if(side1s + side2s == side3s) {
                TriangleType = "Right";

                System.out.println("The Triangle is..." + TriangleType);



            } else if (side1s + side2s > side3s) {
                TriangleType = "Acute";

                System.out.println("The Triangle is..." + TriangleType);

            } else if(side1s + side2s < side3s) {
                TriangleType = "Obtuse";

                System.out.println("The Triangle is..." + TriangleType);

            } else {
                TriangleType = null;

                System.out.println("The Triangle is..." + TriangleType);



            }



        }
    }






}
\$\endgroup\$
1
  • \$\begingroup\$ Consider using arrays instead of side1 side2 side3 etc. Also, try to run System.out.println("The Triangle is..." + TriangleType); just once, not multiple times. \$\endgroup\$
    – user1149
    Apr 29 '16 at 0:31
6
\$\begingroup\$

Bug

Your code assumes that the user will enter the longest side last! This is a bug. Id the user enters the values 10, 1, and 1, it will declare that it is a triangle because 10 + 1 > 1

Functions!

Where are your functions?

One of the best ways to document your code, and make sense of it, is to have well-named functions. Your function is main, and.... well, that does not do a good job of telling us what the function does.

What you need is a function called something like boolean isValidTriangle(int sideA, int sideB, int sideC), then make that function "work".

A valid triangle is one whose shorter sides add up to more than the longest side, right? The trick is finding the longest side.... In this case, recursion is a trick that can help....

boolean isValidTriangle(int sideA, int sideB, int sideC) {

    // recursion trick, make sure sideA is the longest side!

    if (sideA < sideB) {
        // call the function with sideB first
        return isValidTriangle(sideB, sideA, sideC)
    }

    if (sideA < sideC) {
        // call the function wwith sideC first
        return isValidTriange(sideC, sideB, sideA)
    }
    
    // OK, sideA is the longest, for sure!
    return sideA <= sideB + sideC

}

Similarly, you can do the same sort of thing to classify the triangle types.

\$\endgroup\$
3
  • \$\begingroup\$ Ok, thank you so much for the response, I will make sure to add some more functions and fix that bug. Thanks again \$\endgroup\$ Apr 29 '16 at 0:29
  • \$\begingroup\$ Wouldn't it be simpler to hold the sides in an array and simply sort them? \$\endgroup\$
    – Edward
    Apr 29 '16 at 11:43
  • \$\begingroup\$ @Edward - perhaps. It may also be simpler to just do some variable swaps too as a manual sort. Recursion is perhaps overkill. \$\endgroup\$
    – rolfl
    Apr 29 '16 at 12:08
4
\$\begingroup\$
        Scanner input2 = new Scanner(System.in);
        System.out.println(Q2);
        side2 = input2.nextInt();

At this point, you already have a Scanner available. You could just use that rather than create a new one for each input.

        System.out.println("What is the second side of the Triangle?");
        int side2 = input.nextInt();

I would move the declaration of side2 to the point of initialization. You don't need to declare variables at the beginning of a method in Java. Point of initialization is often enough.

Also, I wouldn't name a helper variable Q1. It's easier to just put the string constant. If you do need the variable, either name it something explanatory like SECOND_SIDE_PROMPT or put it in an array. E.g.

        final String [] QUESTIONS = {"What is the first side of the Triangle?",
                "What is the second side of the Triangle?",
                "What is the third side of the Triangle?"};

Then you could iterate over the questions with something like

        List<Integer> sides = new ArrayList<>(3);
        for (String question : QUESTIONS) {
            System.out.println("What is the second side of the Triangle?");
            sides.add(input.nextInt());
        }

But I'm not sure it's worth it for just three questions.

        if(side1 + side2 > side3) {
            Triangle = true;
            System.out.println("The triangle is true!");

        } else {
            Triangle = false;
            System.out.println("The Triangle is False!");
            return;

        }

        if(Triangle == true) {

Since you return in the else, you could write this more simply as

        if (side1 + side2 <= side3) {
            System.out.println("The Triangle is False!");
            return;
        }

        System.out.println("The triangle is true!");

You don't need the Triangle Boolean or the if check. If it's not a valid triangle, you can just return. Then the rest of the method can handle the valid case without further checks.

            side1s = side1 * side1;
            side2s = side2 * side2;
            side3s = side3 * side3;

            if(side1s + side2s == side3s) {
                TriangleType = "Right";

                System.out.println("The Triangle is..." + TriangleType);



            } else if (side1s + side2s > side3s) {
                TriangleType = "Acute";

                System.out.println("The Triangle is..." + TriangleType);

            } else if(side1s + side2s < side3s) {
                TriangleType = "Obtuse";

                System.out.println("The Triangle is..." + TriangleType);

            } else {
                TriangleType = null;

                System.out.println("The Triangle is..." + TriangleType);



            }

Obviously the beginning changes if sides is a List.

            int sumOfSquares = sides.get(0) * sides.get(0) + sides.get(1) * sides.get(1);
            int singleSideSquared = sides.get(2) * sides.get(2);

It seemed easier to take the sum before saving the number.

            String triangleType;
            if (sumOfSquares == singleSideSquared) {
                triangleType = "Right";
            } else if (sumOfSquares > singleSideSquared) {
                triangleType = "Acute";
            } else {
                triangleType = "Obtuse";
            }

            System.out.println("The Triangle is..." + triangleType);

You can both pull out the repeated code and eliminate the last else.

Note that we had to declare triangleType before the if/else block, as declaring it inside would go out of scope before use.

The Java standard is to use camelCase for variable names. StudlyCase is only used for class names.

Consider breaking the whole thing into methods. E.g.

    public static void main(String args[]) {
        List<Integer> sides = fetchSides();
        if (!isValidTriangle(sides)) {
            System.out.println("The Triangle is False!");
            return;
        }

        System.out.println("The triangle is true!");
        System.out.println("The Triangle is..." + determineTriangleType(sides));
    }

Eventually make a Triangle class and move the methods there, but I'm guessing you haven't gotten that far yet.

\$\endgroup\$
1
\$\begingroup\$

Since this is your first program so appreciate your efforts. Although, every time people need to learn more and more so here are few of my suggestions related to your code.

        int side1; int side2; int side3;
        int side1s; int side2s; int side3s; 

This can be simplified to:

int side1 = 0, side2 = 0, side3 = 0, side1s = 0, side2s = 0, side3s = 0;

You do not need to use wrapper classes:

        Boolean Triangle;

instead simple primitive will work too:

boolean Triangle;

These 3 strings can be in an array or List.

        String Q1 = "What is the first side of the Triangle?";
        String Q2 = "What is the second side of the Triangle?";
        String Q3 = "What is the third side of the Triangle?";

like:

String[] questions = new String[]{"SOMETHING","SOMETHING_MORE","MORE_SOMETHING"};

Also, you can simplify this piece of code as well:

            if (side1s + side2s == side3s) {
                TriangleType = "Right";

                System.out.println("The Triangle is..." + TriangleType);


            } else if (side1s + side2s > side3s) {
                TriangleType = "Acute";

                System.out.println("The Triangle is..." + TriangleType);

            } else if (side1s + side2s < side3s) {
                TriangleType = "Obtuse";

                System.out.println("The Triangle is..." + TriangleType);

            } else {
                TriangleType = null;

                System.out.println("The Triangle is..." + TriangleType);


            }

to something like:

            if (side1s + side2s == side3s) {
                TriangleType = "Right";
            } else if (side1s + side2s > side3s) {
                TriangleType = "Acute";
            } else if (side1s + side2s < side3s) {
                TriangleType = "Obtuse";
            } else {
                TriangleType = null;
            }
            System.out.println("The Triangle is..." + TriangleType);

Instead of creating multiple scanner instances, creating one will suffice. Also, do not forget to close Scanner.

e.g.

scanner.close()

Since, Scanner is Closeable, so you should use try-with-resources block to properly close the resource as:

    try (Scanner input = new Scanner(System.in);) {
        System.out.println(Q1);
        triangle.setSide1(input.nextInt());
    }

but keep in mind, once try-with-resources block finishes, it will close the Scanner, so you have to either handle the input for all 3 questions in same block or every time create a new Scanner object. I will recommend something like this:

    try (Scanner input = new Scanner(System.in);) {
        System.out.println(Q1);
        triangle.setSide1(input.nextInt());
        System.out.println(Q2);
        triangle.setSide2(input.nextInt());
        System.out.println(Q3);
        triangle.setSide3(input.nextInt());
    }

Remember, try-with-resources is only available if you are using java7 or greater.

\$\endgroup\$
0
\$\begingroup\$

This is how I'd do it:

import java.util.Scanner;
import java.util.Arrays;

public class Main {

    public static void main(String args[]) {

    // TODO: really limit yourself to integer-sided triangles only?
    // use an array of integers, in part so we can sort it later
    int[] sides = new int[3];

    // only one word changes when we ask for the sides
    String[] ordinals = new String[]{"first", "second", "third"};

    // to store the input
    Scanner input = new Scanner(System.in);

    for (int i=0; i<3; i++) {
        // TODO: check for negative or zero values here?
        System.out.println("What is the "+ordinals[i]+" side of the Triangle?");
        sides[i] = input.nextInt(); 
    }

    // sort the sides
    Arrays.sort(sides);

    // handle the 'false' case first to avoid if-then-else
    if (sides[0]+sides[1]<=sides[2]) {
            System.out.println("The Triangle is False!");
        return;
    }

    // we know the triangle is true, so print it

    // TODO: since we're going to say the triangle is
    // acute/right/obtuse anyway, it seems redundant to says its
    // true
    System.out.println("The triangle is true!");

    // compute the difference between the sum of squares of the
    // two shorter sides and the longest side (we only need the
    // sign)
    int diff = sides[0]*sides[0] + sides[1]*sides[1] - sides[2]*sides[2];

    System.out.print("The Triangle is...");

    if (diff==0) {
        System.out.println("Right");
    } else if (diff < 0) {
        System.out.println("Acute");
    } else {
        System.out.println("Obtuse");
    }

    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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