2
\$\begingroup\$

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) ;

    }
}
\$\endgroup\$

4 Answers 4

3
\$\begingroup\$

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!

\$\endgroup\$
5
  • \$\begingroup\$ Thanks! do you think that userInput() would be a better function name than gimmeNumber() or could I get a suggestion from what you consider it to be a better name? \$\endgroup\$
    – newbie
    Commented Mar 21, 2017 at 21:58
  • 1
    \$\begingroup\$ Yeah, something along those lines. Certainly in a professional context. It doesn't matter too much with programming exercises obviously but best not to fall into the habit if you land a programming job haha! Honestly though, things like refactoring into more methods are more important. \$\endgroup\$
    – Nathan Hoy
    Commented Mar 21, 2017 at 22:02
  • \$\begingroup\$ I used while(true) only to make the testing a little bit more pleasent, avoding re-runs, I had the doubt about ArrayList as well, I'm not too sure the authours were expecting the reader to choose between array and ArrayList, but in the 30 exercises of the chapter Array and ArrayList object it never sujests to use ArrayList even though they were taught. \$\endgroup\$
    – newbie
    Commented Mar 21, 2017 at 22:05
  • \$\begingroup\$ I'll take your word for it on that one haha :P I've just realised that the question say the user inputs 5 numbers...that's the array size you'd declare right there no? Maybe just a funny line at the end of the question throw people off. Thinking about it I'm sure there is a more efficient solution than this but as a beginner, efficiency perhaps isn't on the top of the list of priorities ;) \$\endgroup\$
    – Nathan Hoy
    Commented Mar 21, 2017 at 22:13
  • 1
    \$\begingroup\$ Well, I did use ArrayList <Integer> vector = new ArrayList <Integer> (5) ; which I think creates an ArrayList of 5 elements instead of 10 elements which is the default when you don't put a number inside the parenthesys. I will do the version with a common new int [5] vector, but I honestly read the 9th edition, and took the exercise explanation from the tenth, and they didn't change a thing to clarify between one or the other :) \$\endgroup\$
    – newbie
    Commented Mar 21, 2017 at 22:21
2
\$\begingroup\$

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!

\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$
1
  • \$\begingroup\$ Is this your blog? \$\endgroup\$ Commented Mar 22, 2017 at 6:13
1
\$\begingroup\$
  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:"));

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.