Simple Java array stores and shows not repeated elements entered by user

Question

I made a new version of this which was actually not behaving correctly, because it showed duplicated values, it just didn't add them again, this is the link to that program, someone point at correctly I think that the problem didn't state that student should use an ArrayList, but an Array, so I made this program again with an array, and I think is a lot better now. I would love some feedback, is this good code? I'm following the advices I got in my last post, please check answers. It works very well now and it took me quite a few hours to get it to work properly, but I loved coding it and learnt quite a few things.

---

class TestUserInputDataHandler

import javax.swing.JOptionPane;
/*
 * 7.12 (Duplicate Elimination) Use a one-dimensional array to solve the following
 * problem:Write an application that inputs five numbers, each between 10 and 100,
 * inclusive. As each number is read, display it only if it’s not a duplicate of
 * a number already read. Provide for the "worst case", in which all five numbers
 * are different. Use the smallest possible array to solve this problem. Display
 * the complete set of unique values input after the user enters each new value.
 */
public class TestUserInputDataHandler {
 public static UserInputDataHandler userInputContainer; 
 public static void main(String[] args) {
 final int USER_INPUTS = 5 ;
 //create UserInputDataHandlerObject
 userInputContainer = new UserInputDataHandler() ;
 //ask user to enter five valid int values
 //everytime user inputs a new value, show non-duplicate ones
 for ( int i = 0 ; i < USER_INPUTS ; i++ )
 {
 //let user input data
 getUserInput() ;
 //print not duplicated values entered by user
 printNonDuplicates() ;
 }
 }
 public static void printNonDuplicates(){
 String nonDuplicatedValues = 
 userInputContainer.nonDuplicatedValuesWithFormat();
 JOptionPane.showMessageDialog(null, 
 String.format ("Nonduplicated values:\n%s",nonDuplicatedValues) ); 
 }
 public static void getUserInput(){
 //ask user to enter a new value and store it in a variable
 int newUserInput = Integer.valueOf(JOptionPane.showInputDialog(null, 
 "Enter a value between 10 and 100 inclusive)")) ;
 //let the container handle the new user input
 userInputContainer.handleUserInput(newUserInput);
 }
}

---

class UserInputData

public class UserInputData {
 //userInput field:it must be between 10 and 100 inclusive
 int userInput ;
 //isDuplicated field:false ONLY if number was entered by user once
 boolean isDuplicated ;
 //constructor
 public UserInputData ( int input ) {
 //check if input is valid
 if ( inputIsValid ( input ) ){
 this.userInput = input ;
 this.isDuplicated = false ;
 }else{
 throw new IllegalArgumentException
 ( "Invalid user input (must be between 10 and 100 inclusive" ) ;
 }
 }
 //userInput getter
 public int getNumericData (){
 return userInput ;
 }
 //isDuplicated getter
 public boolean isDuplicated(){
 return this.isDuplicated;
 }
 public boolean inputIsValid ( int input ){
 //constants declaration
 final int UPPER_BOUND = 100 ;
 final int LOWER_BOUNDS = 10 ;
 //return boolean if data is valid or invalid
 if ( input >= LOWER_BOUNDS && input <= UPPER_BOUND ){
 return true ;
 }else{
 return false ;
 }
 }
 public void setAsDuplicated () {
 this.isDuplicated = true ;
 }
}

---

class UserInputDataHandler

public class UserInputDataHandler {
 //Array of not-duplicated valid user input
 UserInputData[] handler ;
 //constants
 final int USER_INPUTS = 5 ;
 //constructor
 public UserInputDataHandler(){
 this.handler = new UserInputData[USER_INPUTS] ;
 }
 //handles user's input 
 public void handleUserInput (int newInput) {
 int storagePos = 0 ;
 //check if number already exists in array
 if ( ( storagePos = numberAlreadyEntered (newInput ) ) >= 0 ) {
 //set contained UserInputData object as duplicated
 this.handler[storagePos].setAsDuplicated();
 }else{
 //create UserInputData object
 UserInputData userInput = new UserInputData(newInput) ;
 //store new UserInputData object @handler's storagePos
 //value needs to be converted to possitive
 this.handler[ Math.abs(storagePos + 1)] = userInput ;
 }
 }
 //find @storagePos where newInput should be stored,or set repeated input as 
 //duplicated
 public int numberAlreadyEntered (int newInput ){
 //variable to go over handler
 int storagePos = 0 ;
 //check if the value was already entered
 while 
 ( //handler[storagePos] is not null 
 this.handler[storagePos] != null &&
 //if handler[storagePos] doesn't have newInput's numerical value
 newInput != this.handler[storagePos].getNumericData() &&
 //if storagePos < USER_INPUTS
 storagePos < USER_INPUTS 
 )
 {
 //check next handler storage position
 storagePos++ ; 
 }
 //the true side of the following if must return @storagePos, where a 
 //duplicate value was found
 if ( this.handler[storagePos] != null && storagePos != USER_INPUTS ){
 return storagePos ;
 }else{
 //return a negative number if null pointer or unreachable position 
 //@storagePos
 return ( -1 * storagePos ) - 1;
 }
 }
 //return non duplicate values
 public String nonDuplicatedValuesWithFormat(){
 //empty string
 String s = "" ;
 //concat string with nonduplicated values
 for ( int i = 0 ;
 this.handler[i] != null &&
 i < USER_INPUTS
 ; i++ )
 {
 //concatenate nonduplicated values
 if ( ! this.handler[i].isDuplicated() ){
 s += this.handler[i].getNumericData() + "\n" ;
 }
 }
 //return concatenated String
 return s;
 }
}

Answer 1

Let's start small with the question: If you want to change your program to take 6 inputs instead of 5, which classes have to change? If your answer includes more than 1 class you're not following the DRY principle. This can be easily fixed by passing it in the constructor

public UserInputDataHandler(int nbUserInputs){
 this.USER_INPUTS = nbUserInputs;
 this.handler = new UserInputData[nbUserInputs]; 
}

Next small thing is the method:

public static void getUserInput(){

What I expect from a getSomething() method is that it returns that something. So instead of also putting it into the userInputContainer this method should just return it. While we're looking at this method, it would also be more logical to make sure that the input we're getting is a valid one. That way the rest of the program doesn't need to be conserned with checking this. So let's steal that inputIsValid method from UserInputData for now and put it next to this one. And then have another that keeps prompting the user until a valid input is given.

public static int getValidUserInput(){
 int userInput = getUserInput();
 while(!inputIsValid(userInput)){
 //optional here is to tell the user it was an invalid input
 userInput = getUserInput();
 }
 return userInput;
}
public static int getUserInput(){
 //ask user to enter a new value and store it in a variable
 return Integer.valueOf(JOptionPane.showInputDialog(null, 
 "Enter a value between 10 and 100 inclusive)")) ;
}
public static boolean inputIsValid ( int input ){
 //constants declaration
 final int UPPER_BOUND = 100 ;
 final int LOWER_BOUNDS = 10 ;
 //return boolean if data is valid or invalid
 return input >= LOWER_BOUNDS && input <= UPPER_BOUND;
 //notice how I removed the if(...){return true} else {return false} here
}

Since these 3 methods logically belong together along with the upper and lower bounds constants, you might as well put them inside their own class. For example a UserInputFetcher class. I'll let you do this on your own.

---

Now for the major part. In my opinion making the user input numbers into a class was a little bit overkill. It's possible to make it work with just an int array

int[] userInputs;
public UserInputDataHandler(int nbUserInputs){
 this.userInputs = new int[nbUserInputs] ;
}

But this requires some other changes as well. We first make it easy for ourselves by introducing a new variable:

int nextNumberIndex = 0;

This one keeps track of what index the next number should be inserted if it's unique. And to know which numbers in this array were input from the user.

The numberAlreadyEntered method will change a bit. First of, I make it return a boolean so it answers the following question: Is this number new or a repeat? Now giving it a better name and changing the implementation results in this:

public boolean isNewNumber(int newInput){
 // the first number we see can't be a repeat
 if(nextNumberIndex < 1){
 return true; 
 }
 //go over the previous unique inputs and see if it's the same number
 for(int i = 0; i < nextNumberIndex; i++){
 if(newInput == userInputs[i]){
 return false;
 }
 }
 //out of for loop so have not seen it before
 return true;
}

But Imus you say, how do we then handle a new user input? Well we do the following:

public void handleUserInput (int newInput) {
 if(!isNewNumber(newInput)){
 return; //duplicate so don't need to do anything here
 }
 userInputs[nextNumberIndex] = newInput;
 nextNumberIndex++;
}

or

public void handleUserInput (int newInput) {
 if(isNewNumber(newInput)){
 userInputs[nextNumberIndex++] = newInput;
 }
}

depending entirely on your preference.

I believe that with this you should be able to change the other methods accordingly. Hint: To print the unique numbers is the same for loop as used in the isNewNumber method.

These changes will probably make the class UserInputData obsolete :)

edit: An example since it wasn't clear yet:

userInputs = {0,0,0,0,0}
nextNumberIndex = 0

The user inputs: 10

userInputs = {10, 0, 0, 0, 0}
nextNumberIndex = 1
output is 10

The user inputs: 12

userInputs = {10, 12, 0, 0, 0}
nextNumberIndex = 2
output is 10, 12

The user inputs: 10

userInputs = {10, 12, 0, 0, 0}
nextNumberIndex = 2
output is 10, 12 again

notice how this input is completely ignored here, because it's a duplicate.

The user inputs: 99

userInputs = {10, 12, 99, 0, 0}
nextNumberIndex = 3
output is 10, 12, 99

The user inputs: 10

userInputs = {10, 12, 99, 0, 0}
nextNumberIndex = 3
output is 10, 12, 99 again

To print our result we use

for(int i = 0; i < nextNumberIndex;i++){
 //handle userInputs[i]
}

This will result in the numbers 10, 12, 99 (at the end) and stop because the next i is equal to nextNumberIndex so the for loop ends. These are exactly the non-duplicat numbers that we wanted.

Good luck with the next version of your solution!

• newbie: Hey! thanks for taking the time for that reply, I think your answer is wrong because you seem to think this can be solved by an array of integers, which is incorrect. If array = { 1, -1, 2} then user inputs 2 the array will be array = { 1, -1, -1} and if user enters 2 one more time it will be put @position 1, or 2, which is wrong!
• Imus: Hmm, I did say "The easiest but kinda quick and dirty way is to assume that the user will only be allowed to enter positive numbers" but looking at the code I gave you that actually doesn't matter. It should all just work even with negative integers.
• newbie: What you are saying will only work if user inputs the same number 2 or 4 times (do you get why?). But I will stick with the only thing that I think you said that makes sense, which is that if I say getInputValue() then that function should return a value. Try to read the problem again.
• Imus: Ah sorry, I forgot to print out the result after each user input. Will update my example accordingly. (sorry for the multiple edits). It should still work tho :)
• Imus: @newbie Note that the userInput array doesn't say that the userInput[2] is the 3d number that the user has input. It might be the 4th if there was a duplicate (like in my example). This doesn't matter because we only need to output all the unique numbers. Perhaps the array name was badly chosen and should be changed to: uniqueUserInputs

Answer 2

Bug

The program doesn't handle well the case when all input values are distinct (no duplicates). This loop condition will throw an exception for accessing this.handler[5] which is out of bounds:

 this.handler[i] != null && i < USER_INPUTS

The fix is simple enough: flip the two sides of &&:

 i < USER_INPUTS && this.handler[i] != null

Comments

Do the comments here tell you something the code doesn't?

//let user input data
getUserInput() ;
//print not duplicated values entered by user
printNonDuplicates() ;

No, these comments are stating the obvious, and serve no purpose.

It's best when the code is so simple and obvious that it doesn't need any comments.

Cause and effect

This kind of code is extremely difficult to work with:

userInputContainer = new UserInputDataHandler() ;
for ( int i = 0 ; i < USER_INPUTS ; i++ )
{
 getUserInput() ;
 printNonDuplicates() ;
}

The problem is that the dependence of the components is not visible. getUserInput takes things (from somewhere), and puts them into userInputContainer. printNonDuplicates then takes things from userInputContainer to print them. But this piece of code doesn't tell this clearly, the reader can only know this by reading their implementations.

The first warning sign was the static userInputContainer. Avoid static variables as much as possible.

Class design

Some of the classes could be better.

• TestUserInputDataHandler: this is the main class that drives everything, and this is more or less fine. Its responsibility is to drive everything, and that's ok. Only the name is very misleading, as Test in a name is often used in unit test classes. The name of the exercise, DuplicateElimination would be better.

• UserInputDataHandler: this class is in charge of storing the user inputs, and checking for duplicates, and formatting. The name "handler" is too generic to be useful, it doesn't really say anything. Something like UniqueNums or NonDuplicatedNums would be better, with a simplified interface and implementation.

Consider this alternative class design:

• UniqueNums, with methods add and format, and a constructor that takes the number of inputs as parameter. The class could encapsulate the storage of the inputs, the handling of duplicates, and format for printing.

• InputReader, with a read method, that would return a valid input, or raise an error.

• OutputWriter, with a print method, that would print output.

• DuplicateElimination with a main method, that would run the show.

The implementation of DuplicateElimination could be:

public class DuplicateElimination {
 public static final int USER_INPUTS = 5;
 public static void main(String[] args) {
 UniqueNums nums = new UniqueNums(USER_INPUTS);
 InputReader reader = new InputReader();
 OutputWriter writer = new OutputWriter();
 for (int i = 0; i < USER_INPUTS; i++) {
 nums.add(reader.read());
 writer.print(nums.format());
 }
 }
}

Implementation details about reading from or writing to a graphical user interface are gone. The interaction between the other classes is directly visible: you add numbers to nums, that were read from reader, and nums also provides the formatted values to print using the writer. The names are also short and simple, easy to read and understand.

The implementation of InputReader and OutputWriter are not very interesting, you could copy the code from your original.

Here's one way to implement UniqueNums, and then I'll show another way:

class UniqueNums {
 private final int[] nums;
 private final boolean[] duplicated;
 private int nextIndex = 0;
 public UniqueNums(int count) {
 nums = new int[count];
 duplicated = new boolean[count];
 }
 public void add(int num) {
 for (int i = 0; i < nextIndex; i++) {
 if (nums[i] == num) {
 duplicated[i] = true;
 return;
 }
 }
 nums[nextIndex++] = num;
 }
 public String format() {
 StringBuilder sb = new StringBuilder();
 for (int i = 0; i < nextIndex; i++) {
 if (!duplicated[i]) {
 sb.append(nums[i]).append(" ");
 }
 }
 return sb.toString();
 }
}

Notice that all the names are short and simple, yet expressive. This class is in charge of storing the inputs and tracking duplicates, using whatever strategy. If tomorrow you want to reimplement storage using an ArrayList instead of an array (or best using a LinkedHashMap), you could do so by touching only this class, and in a fairly straightforward way.

If instead of an int[] and boolean[] to track the inputs and duplicates you prefer to use a dedicated class, that's possible too, by changing only this class, without touching the rest of the program:

class UniqueNums {
 private static class Num {
 private final int value;
 private boolean duplicated;
 private Num(int value) {
 this.value = value;
 }
 public int getValue() {
 return value;
 }
 public boolean isDuplicated() {
 return duplicated;
 }
 public void markDuplicated() {
 duplicated = true;
 }
 }
 private final Num[] nums;
 private int nextIndex = 0;
 public UniqueNums(int count) {
 nums = new Num[count];
 }
 public void add(int num) {
 for (int i = 0; i < nextIndex; i++) {
 if (nums[i].getValue() == num) {
 nums[i].markDuplicated();
 return;
 }
 }
 nums[nextIndex++] = new Num(num);
 }
 public String format() {
 StringBuilder sb = new StringBuilder();
 for (int i = 0; i < nextIndex; i++) {
 if (!nums[i].isDuplicated()) {
 sb.append(nums[i].getValue()).append(" ");
 }
 }
 return sb.toString();
 }
}

Neither implementation is really better than the other. It doesn't even matter much which one is "better". The important point is that you can easily change the internal implementation of a component without changing the rest of the program.

• newbie: UserInputData: this class contains a single user input, and has methods to check if an input is valid and if an input is duplicate. This class is simply unnecessary. Why? A single user input is a simple integer. The validation of the input would be better in the class that is in charge of reading the input, so not here. If an input is duplicate, it can be simply ignored, there's no need to store the duplicate state anywhere. so how do you know you must ignore a number if you don't keep track of it's duplicate boolean state? you are not eliminating any number.You are just not adding it.
• newbie: I made a similar version of what you are saying,but is wrong,in my previous version link, (it's kinda annoying to explain everything again, when it's actually very clear in my OP),I might have comment too much,it's just the way the book name and comment everything,TestUserInput,etcetera,class TestUserInput exists cause I needed it,and the book I'm readingis explains inheritance and polymorfism after this chapter,that's why my code is funny in places.
• newbie: When it says TestUserInput exists because I needed it, it's suppose to say UserInputData exists because I need it, without that class, you can not know when a number is duplicate... and you have to show every nonduplicate number that was read, right after a new one is read. You just can't do that with an array of integers unless you do something not very elegant with many variables.
• janos: @newbie Based on my better understanding of the exercise, I rewrote my answer. I hope you'll find it useful.
• newbie: You are right aboutthis.handler[i] != null && i < USER_INPUTS even though it works very well, I don't know why, but it should throw that indexOutOfBoundException. The comments stating the obvious, are made by me before coding and then I don't erase them, because that's the way the book I'm reading does, thanks for stating that code looks more clear without them, and I started coding today only making comments when it's not so obvious.