HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Finding repeating numbers in an array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arraynumbersrepeatingfinding

Problem

I want to search through an array of n numbers and find the numbers that are repeated. So far I have this code, which does the job, but I find it to be a rather cumbersome method, but I can't seem to find another method of doing it.

class Checknumber{
    int [] numbers = new int [5];
    Scanner inn = new Scanner(System.in);
    boolean b = true;
    int temp = -1;

    void sjekk(){
        System.out.println("Write five numbers");
        for(int i = 0; i < numbers.length; i++){
        numbers [i] = inn.nextInt();
    }

    //sjekker om det er like tall i arrayen
    System.out.print("\nNumbers that are repeated: ");
    for(int i = 0; i < numbers.length; i++){

        if(!b){
            System.out.print(temp + " ");
        }

        b = true;
        temp = numbers[i];
        for(int j = 0; j < numbers.length; j++){
            if(i != j && temp == numbers[j] && numbers[j] != -2){
                b = false;
                numbers[j] = -2;
            }
        }
    }

}

Solution

Others have given alternate solutions. However, since this is Code Review, I'll offer a critique instead, because I think you will learn more from that than by just seeing the answer.

Interfaces

  • Checknumber is too vague. I suggest DuplicateNumberDetector.



  • In your interfaces (class and method names), pick either English or Norwegian and stick with it. For your comments, use whatever language works for you.



  • sjekk() (meaning "check") is actually responsible for receiving input rather than performing the checking. How misleading!



Variables

  • b is very poorly named. I suggest unique instead.



  • temp is somewhat poorly named. I suggest num instead.



  • Only older dialects of C require you to declare all variables at the top. Java lets you declare variables near the point of first use. You should take advantage of that. int temp should be declared inside the for (i) loop, since it is never used outside the loop.



  • You might try to pull the declaration of b inside the for (i) loop too. But then you will realize that b refers to the uniqueness of the previous numbers[i]. That reveals a bug: what if the last two numbers are identical (numbers[3] == numbers[4], but numbers[2] != numbers[3])? You haven't handled the termination case correctly. Hint: Move your print statement. The lesson to be learned here is that declaring temporary variables far from the point of use is almost as evil and dangerous as using global variables.



Logic

  • You are using -2 as a special value to indicate that an array element has already been detected as a duplicate. That's bad because your code will fail if -2 happens to be one of the inputs.



  • Overwriting the input array is a surprising side effect. That's as unforgivable as sending your computer to a repair shop to install a video card and getting it back with the hard drive reformatted. If you really need to overwrite the input, state so clearly in a comment. You should be able to find a solution to this problem that does not require overwriting.



  • Your inner loop has j going from 0 to the end of the array. That means you are handling every pair twice. You should probably be able to start from i + 1 instead, to reduce your processing in half. Handling each pair only once should simplify your logic as well.



Efficiency

  • You have two for loops, each iterating over the entire array. If the array has n elements, then the run time for your algorithm is O(n2). (Even with the inner-loop optimization mentioned in the previous point, it would still be O(n2).) For a small homework problem like this, that is perfectly acceptable, because simplicity is the main goal. If you wanted to handle very large arrays, though, it would be more efficient to sort the input first or use a HashMap.



Separation of concerns

  • Your input, processing, and output code are all intertwined. Get into the habit of separating them. Your code will be more reusable and easier to understand.



.

public class DuplicateNumberDetector {  
    int[] promptInputs(int numberOfInputs) { ... }

    // If the order doesn't matter, use Set instead
    List findDuplicates(int[] numbers) { ... }

    void printDuplicates(List dups) { ... }

    public static void main(String[] args) {
        printDuplicates(findDuplicates(promptInputs(5)));
    }
}

Code Snippets

public class DuplicateNumberDetector {  
    int[] promptInputs(int numberOfInputs) { ... }

    // If the order doesn't matter, use Set<Integer> instead
    List<Integer> findDuplicates(int[] numbers) { ... }

    void printDuplicates(List<Integer> dups) { ... }

    public static void main(String[] args) {
        printDuplicates(findDuplicates(promptInputs(5)));
    }
}

Context

StackExchange Code Review Q#18936, answer score: 13

Revisions (0)

No revisions yet.