Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Write a function that returns an array of integers with 1000 elements containing the values 1 to 1000 in random order. No number can be repeated or omitted.Your answer should show good programming style, technique and attention to accuracy and performance.

Hint: use Random rnd=new Random() to create a new instance of the random number generator and rnd.Next(1,1000) to get a random integer value between 1 and 1000. No other .Net framework classes should be needed outside of the intrinsic data types (i.e. DO NOT use collections).

For that I have developed the following code:

import java.util.Random;

public class Domain {

public int[] randomIntArray(int size) {

    Random random = new Random();
    int intArrayHolder[] = new int[size];

    for (int i = 0; i < intArrayHolder.length; i++) {

        boolean isDuplicate = false;

        /*
         * because random.nextInt(int var) return between 0 and given-no and
         * we do not want 0. So size +1 and later in program, we do not
         * assign 0 to the intArrayHolder.
         */

        int randomNextInt = random.nextInt(size + 1);

        for (int j = 0; j < i; j++) {
            if (intArrayHolder[j] == randomNextInt) {

                isDuplicate = true;
                break;
            }
        }

        if ((!isDuplicate) && (randomNextInt != 0)) {
            intArrayHolder[i] = randomNextInt;
        } else {

            i--;

        }

    }

    return intArrayHolder;
}

}

JUnit test class:

import java.util.HashSet;
import java.util.Set;

import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

public class DomainJunitTest {

Domain domain = null;
int arraySize = 0;

@Before
public void initialize() throws Exception {
    domain = new Domain();
    arraySize = 1000;
}

@Test
public void randomIntArray_size_check() {

    int intArrayHolder[] = domain.randomIntArray(arraySize);

    Assert.assertEquals(arraySize, intArrayHolder.length);
}

@Test
public void randomIntArray_duplicate_value_check() throws Exception{

    int intArrayHolder[] = domain.randomIntArray(arraySize);

    Set<Integer> intSet = new HashSet<>();

    boolean isDublicate = false;

    for (int i = 0; i < intArrayHolder.length; i++) {

        if (intSet.contains(intArrayHolder[i])) {
            isDublicate=true;
        }else{
            intSet.add(intArrayHolder[i]);
        }
    }

    Assert.assertEquals(false, isDublicate);

}

@Test
public void randomIntArray_size_check_after_removing_dublicate() {

    int intArrayHolder[] = domain.randomIntArray(arraySize);

    Set<Integer> intSet = new HashSet<>();

    for (int i = 0; i < intArrayHolder.length; i++) {
        intSet.add(intArrayHolder[i]);
    }

    Assert.assertEquals(arraySize, intSet.size());

}

}

And I am being advised it is not good enough. Can someone point out where I went wrong?

share|improve this question
2  
Are you allowed to make use of java.util classes? You could simply create a list/set of 1000 integers in ascending order and call Collections.shuffle() to randomize the list/set. It's no more than 5 lines of code. –  GiantTree 21 hours ago
3  
@GiantTree - that's a good suggestion, although Collections.shuffle() would need to have Object Integers, not primitive int values - though some 'simple' manipulations would get you there. –  rolfl 21 hours ago
1  
@rolfl alternatively a simple Fisher-Yates (swap) algorithm would do the trick. –  GiantTree 21 hours ago
    
@GiantTree, yes. Now, I see the point they are asking this --> Fisher-Yates (swap) algorithm Simple short easy to understand. –  yas 21 hours ago
2  
@GiantTree - as an aside, I am almost convinced that Arrays does not have 'shuffle' methods just so that teachers have something they can add to their courses ;-) –  rolfl 20 hours ago

2 Answers 2

up vote 11 down vote accepted

Your approach is not incorrect. It will get the job done (albeit, slower). If you want to improve your approach, keep reading. But first a few points on your code:

Points on Your code

  1. intArrayHolder seems a bit wordy to me. I believe intArray or even array would serve your purpose, without sacrificing readibility. Same for randomNextInt.
  2. for (int i = 0; i < intArrayHolder.length; i++) is redundant because the length of intArrayHolder will be equal to size owing to the fact that you declared it like so. for (int i = 0; i < size; i++) will serve you better.
  3. Please note that random.nextInt(size + 1) does not return an integer between 1 and 1000. It returns an integer in the range \$[0, 1001)\$, i.e. a number \$x\$ such that \$0\leq x < 1001\$. If you do a count, you will find that you are actually getting \$1001\$ elements. The extra number is \$0\$. To fix it, you must write int randomNextInt = random.nextInt(size) + 1; It is quite easy to understand: you just add a \$1\$ to shift the range from \$[0,1000)\$ to \$[1,1001)\$, i.e. the correct range.
  4. ((!isDuplicate) && (randomNextInt != 0)) can be shortened to !(isDuplicate || ramdomNextInt == 0), by using De Morgan's laws. It would be even better if you flip the condition and force next iteration before adding the element to the array.
  5. A general advice is to update the loop counter only once. People generally estimate the number of times the loop will execute by stealing a glance at the for (... statement. A while loop is preferred when the number of iterations is unknown or updation is irregular.
  6. You seem to be using snake_case along with camelCase. The standard in Java is to use camelCase.

Therefore, the refactored code would be:

import java.util.Random;

public class Domain {

    public int[] randomIntArray(int size) {

        Random random = new Random();
        int array[] = new int[size];

        int i = 0;

        while (i < size) {
            boolean isDuplicate = false;
            int randomInteger = random.nextInt(size) + 1;

            for (int j = 0; j < i; j++) {
                if (array[j] == randomInteger) {
                    isDuplicate = true;
                    break;
                }

            }
            if (isDuplicate) {
                continue;
            } else {
                array[i++] = randomInteger;
            }

        }
        return array;
    }
}

A Better Solution

Your solution's best case running time is \$\Omega(n^2)\$. Moreover, as Boris the Spider pointed out: "... there is absolutely no guarantee that Random.nextInt() won't decide to return 1 for ever and ever. ..." Therefore, it might be possible that your code never terminates (although it will be very unlikely). You can make it faster in the following manner:

import java.util.Random;

public class Domain {

    public int[] randomIntArray(int size) {

        Random random = new Random();
        int array[] = new int[size];
        // populate the array with sequential values
        for (int i = 0; i < size; i++) {
            array[i] = i + 1;
        }
        // "shuffle" the array
        for (int i = 0; i < size; i++) {
            int j = random.nextInt(size - i) + i;
            // swap
            int temp = a[i];
            a[i] = a[j];
            a[j] = temp;
        }
        return array;
    }
}

It's running time is linear i.e. \$\Theta(n)\$1. I am basically describing the Fisher-Yates shuffle

Some Background Info: The Big-O stuff

The Big-O notation is used to define an upper limit of the number of operations executed (the maximum running time, in simpler terms). For example, if the exact number of iterations based on a given parameter (like the number of elements, digits, etc) called \$n\$ is $$2n^2+3n-4$$ we can say that a reasonable upper limit is \$3n^2\$. In the Big-O notation, the constant term gets absorbed. When we represent this in the Big-O notation, we say: "the (maximum) running time is proportional to \$O(n^2)\$".

People tend to forget the other two notations: the Big-Omega (\$\Omega\$) and the Big-Theta (\$\Theta\$) The former defines a lower limit and the latter is the Big-O and the Big-Omega combined together. In the example above, the lower limit can be \$2n^2\$. Absorbing the constant, we say:"the (minimum) running time is proportional to \$\Omega(n^2)\$". The function can therefore be bounded within both upper and lower limits, or both the upper and the lower limits are in the order of \$\Theta(n^2)\$.

Your code has no upper limit. It only has a lower limit. So, only the \$\Omega\$ is defined for your code (unless we set out to do a lengthy mathematical analysis of the behaviour of the nextInt() function).

I suggest that you read the related Wikipedia articles. (I didn't cover \$o, \omega\$ and \$\theta\$, that's your homework). Remember, Google is your friend!


1 Assuming that the nextInt() method takes \$O(1)\$ (i.e. constant) time to execute.

share|improve this answer

I'm not fluent with Java, I'll let you complete what is commented in this code.

int intArrayHolder[] = new int[size];
int randomNextInt = /* generate a number between 1 and 1000 */;

// Fill the array with ordered values
for (int i = 0; i < intArrayHolder.length; ++i) {

    while (/* intArrayHolder[randomNextInt] is already set */) {
        randomNextInt = (randomNextInt + 1) % 1000 + 1; // Or you can generate another random number, but it will take a lot longer
    }

    intArrayHolder[randomNextInt] = i;

    randomNextInt = /* generate a number between 1 and 1000 */;
}

In your code, if your array is filled with 999 values, you have 1/1000 chances to generate the only value possible left. This can take a huge amount of time to find the last possible value. The more your array gets filled, the more time it will take to place the next value.

This code will only generate 1000 random numbers, instead of generating another random number each time a duplicate is found.

Also, the variable i is not used to determine where we will place the random value, but it contains the value to be placed in the array. This simplifies the way you can handle collisions (this way, you are not forced to generate another random value at each collision).

share|improve this answer
1  
This seems over complicated. The standard approach is 1) generate ordered numbers 2) shuffle. –  Boris the Spider 18 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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