Problem
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: tt" + numberOfUnique);
outputStatements(uniqueNumber);
} else if (uniqueCheck == false) {
System.out.println(input + " is valid, but not unique");
System.out.println("Unique so far: tt" + 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();
}
}
Solution
My three main remarks are:
- You call
isUnique()incorrectly, causing a bug. (ThevalidNumberarray never gets populated.) - 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.
-
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: tt" + numberOfUnique);
printArray(uniqueNumbers, numberOfUnique);
}
}
public static void main(String[] args) {
// Anything that can be parameterized should be done so, for
// flexibility.
run(5);
}
}
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.
-
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; }