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

Question

Is this good code? I do believe I improved quite a lot since last year, still I would really appriciate some feedback :)

import javax.swing.JOptionPane;
import java.util.ArrayList ;

public class IfNotRepeated {

    /*
     *  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.
     */

    final static int ELEMENTS = 5 ;
    final static int UPPER_BOUND = 100 ;
    final static int LOWER_BOUND= 10 ;


    public static void main(String[] args) {


        int number = 0; //user's input

        while ( true ) //allows the input of numbers which are bounded between 10 and 100 encompass,
            //whenever this boundary is violated, the program ends, without showing any information.
        {

            ArrayList <Integer> vector = new ArrayList <Integer> (5) ; //Array of not repeated user inputs', I need to get a new one
            //every time the user had the chance to input ELEMENTS numbers

            for ( int i = 0 ; 
                    i < ELEMENTS 
                    && ( number = gimmeValue() ) >= LOWER_BOUND 
                    && number <= UPPER_BOUND 
                            ; i++ ) // iterates as long as there are less than ELEMENTS numbers per array, and the boundary is respected
            {

                if ( !vector.contains(number) )
                    vector.add(number) ; //adds a number if the legit value entered hasn't been previously entered (in this serie).

                String elements = "" ;

                for ( int notRepeatedElement : vector )
                    elements += String.valueOf(notRepeatedElement) + '\n' ; //concatenates not repeated elements leaving a new line at the end

                JOptionPane.showMessageDialog(null, elements); //show every non-repeated element

            }
            if ( number < LOWER_BOUND || number > UPPER_BOUND )
                break ; // if out bounds, exit program
        }
    }

    public static int gimmeValue () //shows an input dialog for the user to enter a number and returns that value as an Int
    {

        String number = JOptionPane.showInputDialog("Enter a number between 10 and 100 encompass:") ;
        return Integer.valueOf(number) ;

    }
}

Answer 1

Personally, I'd refrain from putting the bulk of your logic into the main method. I'd refactor the code out into other methods and just call them from the main method.

Also, I wouldn't consider 'while(true)' to be good programming practise with some exceptions. If I were you I'd get the ArrayList size and use a loop that runs up to that. This also removes the need for that if-break statement you have at the bottom of the while loop.

I would also note that the question outlined in the comments says to use an array, not an ArrayList which may or may not be important but given the student/interview style of the question it may need to be a normal array as specified...strictly speaking an ArrayList is an object that behaves 'like' and array.

In regards to comments although they are thorough I'd advise against writing comments on the same line as statements (unless they are very short) as it is inconvenient for anyone reviewing your code to have to scroll left and right just to read the comments.

Variable names could be a touch better too but this only really applies in a professional context where other people are going to be looking at your work (same as comments I suppose). Rather that use 'number' as variable name use something a bit more descriptive like 'usersNumber' or something like that...all goes towards easier understanding and readability. I should also note that this applies to method names too (eg. gimmeValue doesn't really make much sense).

However with all that being said, keep it up! Programming is all about refinement and practise and no doubt you'll be an excellent, clean coder in no time!

Answer 2

I think @Nathan Hoy already commented the most important points.

I would add:

It's a good practice to use the interface(List) to declare the variable type:

List <Integer> vector = new ArrayList<Integer>(5);

Using brackets within your if's (even if they are one line statements) will improve your code readability.

if (!vector.contains(number))
    vector.add(number);

vs

if (!vector.contains(number)) {
    vector.add(number);
}

Keep it up, you'll be an excellent dev!

Answer 3

Can you just variate you test and use ArrayList.remove(object) . Just variating a little bit can help to understand where the issue originates. This is just for your case. Otherwise beware of using Java ArrayList remove() in production code, it is very inefficient. This advice is very useful: ArrayList performance

Answer 4

1. Missing curly braces on if and for loops
2. You could use StringBuffer instead of String elements as concatenations are performed on the elements.
3. Not putting all the code in main method. Instead of that extract the business logic into a separate method and call it from main
4. In the gimmeValue() method you can directly return the value and no need to create String number. e.g. Instead of -

String number = JOptionPane.showInputDialog("Enter a number between 10 and 100 encompass:") ; return Integer.valueOf(number) ;

replace with

return Integer.valueOf(JOptionPane.showInputDialog("Enter a number between 10 and 100 encompass:"));