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!

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.