1
\$\begingroup\$

This is an assignment for an intro to Java course. It works with the test case given, but I would like to know if there is anything / a lot of things that should be different or could be better.

Specification:

Write a Java application that accepts 5 “valid” numbers from the user.

A valid number is between 50 and 100 inclusive. If the user enters a number outside of the valid range, an error message should indicate this. An invalid number is not counted as one of the 5. The user might enter 50 invalid numbers, and the program will keep asking for numbers between 50 and 100.

If the user enters a number that they have previously entered, this is counted as one of the 5. A message appears that it is not unique. A unique number is a valid number that has not been entered up to this point. A non-unique number is a valid number that has already been entered.

After each valid number is entered, the count of the unique numbers entered so far is printed, and all of the unique numbers are printed on a single line. (see example)

When the user has entered 5 valid numbers, a message appears informing the user of how many unique, valid numbers were entered, and the unique numbers are printed again.

package partB;

import java.util.Scanner;

public class PartB {

    public static void main(String[] args) {

        int[] validNumber;
        int[] uniqueNumber;
        int input, numberOfUnique = 0;
        boolean validCheck, uniqueCheck;
        validNumber = new int[5];
        uniqueNumber = new int[5];

        Scanner userInput = new Scanner(System.in);

        for (int i = 0; i < validNumber.length; i++) {

            System.out.print("\nEnter an integer: \t");
            input = userInput.nextInt();

            validCheck = isValid(input);

            while (validCheck == false) {
                System.out.print("\nEnter an integer: \t");
                input = userInput.nextInt();
                validCheck = isValid(input);
            }

            if (validCheck == true) {

                uniqueCheck = isUnique(validNumber, input);
                validNumber[i] = input;
                if (uniqueCheck == true) {
                    numberOfUnique++;
                    uniqueNumber[i] = input;
                    System.out.println("Unique so far: \t\t" + numberOfUnique);
                    outputStatements(uniqueNumber);

                } else if (uniqueCheck == false) {
                    System.out.println(input + " is valid, but not unique");
                    System.out.println("Unique so far: \t\t" + numberOfUnique);
                    outputStatements(uniqueNumber);
                }
            }
        }
        System.out.println();
        System.out.println("You entered " + numberOfUnique
                + " unique valid numbers!");
        outputStatements(uniqueNumber);
    }

    public static boolean isValid(int validParameter) {
        boolean result;

        if (validParameter >= 50 && validParameter <= 100) {
            result = true;
        } else {
            result = false;
            System.out.println("The number " + validParameter
                    + " is out of the valid range.");
        }
        return result;
    }

    public static boolean isUnique(int[] array, int uniqueParamter) {
        boolean result = true;

        for (int i = 0; i < array.length; i++)
            if (array[i] != 0) {
                if (array[i] == uniqueParamter) {
                    result = false; // is NOT unique
                    break;
                } else {
                    result = true; // is unique

                }
            }
        return result;
    }

    public static void outputStatements(int[] array) {

        for (int i = 0; i < array.length; i++) {
            if (array[i] != 0) {
                System.out.print(array[i] + " ");
            }
        }
        System.out.println();
    }
}
\$\endgroup\$
1
\$\begingroup\$

My three main remarks are:

  1. You call isUnique() incorrectly, causing a bug. (The validNumber array never gets populated.)
  2. You use boolean variables gratuitously. None of them is necessary to solve this exercise; you should be able to improve your code by eliminating all of them.
  3. The top of your for-loop has repetitive code that could be better written as a do-while loop.

    for (int i = 0; i < validNumber.length; i++) {
        do {
            System.out.print("\nEnter an integer: \t");
            input = userInput.nextInt();
        } while (!isValid(input));
        // etc.
    }
    

    See my solution below for an even better approach.

I also have a lot of minor critiques, which I have included below as comments inline.

package PartB;
import java.util.Scanner;

public class PartB {

    public static boolean isValid(int num) {
        // A function should do one thing only!  Printing a message would be
        // an unexpected side effect, especially considering the name of
        // the function.

        // No need to use a boolean variable; just return.
        return num >= 50 && num <= 100;
    }

    // I've changed isUnique() to find() because the latter feels more
    // generalized and reusable.
    /**
     * Tests if num is among the first arraySize elements of the array.
     */
    public static boolean find(int num, int[] array, int arraySize) {
        // Pass in the arraySize explicitly to avoid treating 0 as a
        // magic value.  (In this example, using 0 to signify that the
        // array element is still unused happens to work, because 0 is
        // outside the valid range anyway.  But maybe Part C of the exercise
        // will ask you to modify the valid range, and you might accidentally
        // introduce a bug.  Therefore, it's best to code defensively and
        // make the end of the array explicit.)
        for (int i = 0; i < arraySize; i++) {
            if (array[i] == num) {
                // No need to set a result and break.  Just return.
                return true;
            }
        }
        return false;
    }

    // I've renamed outputStatements() to printArray(), because that's
    // what it does.
    public static void printArray(int[] array, int arraySize) {
        // Again, make the end of the array explicit.
        for (int i = 0; i < arraySize; i++) {
            System.out.print(array[i]);
            System.out.print(' ');
        }
        System.out.println();
    }

    public static void run(int size) {
        // Plural names feel more natural for these arrays.
        // You don't need to store validNumbers.
        int[] // validNumbers = new int[size],
              uniqueNumbers = new int[size];
        int   numberOfValid = 0,
              numberOfUnique = 0;

        Scanner userInput = new Scanner(System.in);

        // By not blindly incrementing i each time through the loop,
        // you can change its purpose, from "one iteration per valid
        // number" to "one prompt per iteration".  That way, you can
        // eliminate one layer of loop nesting.
        while (numberOfValid < size) {
            // Prompt
            System.out.print("\nEnter an integer: \t");
            int input = userInput.nextInt();

            // Check validity
            if (!isValid(input)) {
                System.out.println("The number " + input + " is out of the valid range.");
                // When things aren't right, skip to where you want to be
                // as soon as possible.  Don't shilly-shally by setting
                // boolean variables to get there.
                continue;
            }
            // You don't need to store validNumbers
            // validNumbers[numberOfValid++] = input;
            numberOfValid++;

            // Check uniqueness
            if (!find(input, uniqueNumbers, numberOfUnique)) {
                uniqueNumbers[numberOfUnique++] = input;
            } else {
                System.out.println(input + " is valid, but not unique");
            }

            // The following two statements are common to both branches,
            // so pull them out of the if-else.
            System.out.println("Unique so far: \t\t" + numberOfUnique);
            printArray(uniqueNumbers, numberOfUnique);
        }
    }

    public static void main(String[] args) {
        // Anything that can be parameterized should be done so, for
        // flexibility.
        run(5);
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Why would you continue to use an array and not a set? A set gives you the unique test (Set#add returns true if element already existed) and removes the need for the arraySize parameter. \$\endgroup\$ – JohnMark13 Sep 25 '13 at 8:12
  • \$\begingroup\$ @JohnMark13 Good point about using Set. A LinkedHashSet could even preserve the order in which the entries are added. \$\endgroup\$ – 200_success Sep 25 '13 at 9:01
  • \$\begingroup\$ @JohnMark13 This is a homework assignment and we have not discussed sets yet. Looks like that comes way later. I will read up on them. \$\endgroup\$ – jgasko Sep 25 '13 at 19:32
  • \$\begingroup\$ @200_success thanks for all the input, I used the method names because they were given in the assignment. I appreciate the feedback as I know the code was not right but could not figure it out. \$\endgroup\$ – jgasko Sep 25 '13 at 19:37
1
\$\begingroup\$

It looks alright, but there are some improvements you can do. First of all, you can still split your program into smaller functions still, which really helps readability.

Also, something glaring to me is that your functions in "main" are acting as instance variables for the rest of the class, which should not be the case. You should choose where you declare your variables based on the necessity of their scope. If you only use the input int as a store from user input, it should only be declared in the method that gets input from the user.

For instance, you could omit the input int from main and declare it locally in the GetInputFromUser method.

In main:

for (int i = 0; i < validNumber.length; i++) {
     GetInputFromUser(validNumber, uniqueNumber);
}

The method:

public static void GetInputFromUser(int[] validNums, int[] uniqueNums) {
     int input = userInput.nextInt()
     while (!isValid(input)) {
          System.out.print("\nEnter an integer: \t");
          input = userInput.nextInt();
     }
     //Code ommitted

}

I didn't really have time to read all of the specifications carefully, but I hope this helped a bit! Also, you may want to declare the int arrays as instance variables to avoid passing them around all the time.

\$\endgroup\$
0
\$\begingroup\$
  • Comparisons of booleans:

    Please use

    if (validCheck)
    

    or

    if (!validCheck)
    

    instead of

    if (validCheck == true)
    

    or

    if (validCheck == false)
    
  • Scope:

    Define variable in the smallest possible scope, for instance:

    int input = userInput.nextInt();
    
  • Avoid useless comparisons:

    if (uniqueCheck == true) {
        foo();
    } else if (uniqueCheck == false) {
        bar();
    }
    

    could and should be written as:

    if (uniqueCheck) {
        foo();
    } else {
        bar();
    }
    
  • Giving variables their final results as soon as possible:

    boolean result;
    if (validParameter >= 50 && validParameter <= 100) {
        result = true;
    } else {
        result = false;
        System.out.println("The number " + validParameter
                + " is out of the valid range.");
    }
    

    could be written as:

    boolean valid = (validParameter >= 50 && validParameter <= 100);
    if (!valid) {
        System.out.println("The number " + validParameter
                + " is out of the valid range.");
    }
    
  • Return when you can and remove useless variables:

    public static boolean isUnique(int[] array, int uniqueParamter) {
        boolean result = true;
    
        for (int i = 0; i < array.length; i++)
            if (array[i] != 0) {
                if (array[i] == uniqueParamter) {
                    result = false; // is NOT unique
                    break;
                } else {
                    result = true; // is unique
                }
            }
            return result;
        }
    

    could be written as:

    public static boolean isUnique(int[] array, int uniqueParamter) {
        for (int i = 0; i < array.length; i++)
            if (array[i] != 0 && array[i] == uniqueParamter) {
                return false; // is NOT unique
            }
        return true;
    }
    
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the quick reply. It was the isUnique method that I knew was off, but could not see how to simplify it. The other comments are just icing on the cake as I did not know most of those changes were even possible. \$\endgroup\$ – jgasko Sep 24 '13 at 18:35

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.