3
\$\begingroup\$

I have written the code to concatenate two arrays . Please let me know of any loopholes in the written code.

import java.util.Random;
import java.util.Scanner;

public class ConcatArr {
    static Random rand = new Random();
    static Scanner in = new Scanner(System.in);
    public static void main(String[] args){
        System.out.println("Main Method Started");
        System.out.println("Initialize the 1st array");
        int[] arr1 = initArr();
        System.out.println("The element of the 1st array");
        print(arr1);
        System.out.println("Initialize the 1st array");
        int [] arr2 = initArr();
        System.out.println("The element of the 2nd array");
        print(arr2);
        int[] contArr = concatArr(arr1,arr2);
        System.out.println("Concatnated array");
        print(contArr);
        System.out.println("Main Method Ended");
    }
    public static void print(int[] arr){
        System.out.print("{");
        for(int val:arr){
            System.out.print(val + "  ");
        }
        System.out.print("}");
        System.out.println();
    }
    public static int[] initArr(){
        System.out.println("Enter the limit of the array");
        int limit = in.nextInt();
        System.out.println("Enter the maximum value of the array");
        int maxValue = in.nextInt();
        int[] arr = new int[limit];
        for(int i=0;i<limit;i++){
            arr[i] = rand.nextInt(maxValue);
        }
        return arr;
    }
    public static int[] concatArr(int[] arr1,int[] arr2){
        int[] tempArr = new int[arr1.length+arr2.length];
        for(int i=0;i<arr1.length;i++){
            tempArr[i] = arr1[i];
        }
        for(int j=0;j<arr2.length;j++){
            tempArr[j+arr1.length] = arr2[j];
        }
        return tempArr;
    }
}
\$\endgroup\$

4 Answers 4

4
\$\begingroup\$

Use the native method arraycopy as opposed to manually copying the elements for your concatenation method for better performance:

public static int[] concat(int[] a, int[] b) {
   int aLen = a.length;
   int bLen = b.length;
   int[] c= new int[aLen+bLen];
   System.arraycopy(a, 0, c, 0, aLen);
   System.arraycopy(b, 0, c, aLen, bLen);
   return c;
}

Also think about making more use of enhanced for loops, how come you are only using them for part of your solution and not the rest?

for(int i:arr){
    i = rand.nextInt(maxValue);
}
\$\endgroup\$
4
\$\begingroup\$

The fact that you are reusing Random is very good.


You shouldn't shorten variable or method names. Sam the Maintainer will thank you for naming your variables and methods with meaningful name where he/she can graps at first glance what is going on.


One wouldn't expect to see requests for user input inside a method whichs purpose is to initialize an array. The limit and the maximum value should be passed as parameters to this method.

Also note that Random.nextInt(max) returns random values between 0 (inclusive) and max (exclusive).


If you are outputting states to the user you should check the plural form and also the spelling.

So

System.out.println("The element of the 1st array");

should be

System.out.println("The elements of the 1st array");  

and

   System.out.println("Concatnated array");  

should be

   System.out.println("Concatenated arrays");  

In the print() method you can combine the last to instructions

    System.out.print("}");
    System.out.println();  

to

    System.out.println("}");

You should leave your variables and operators some space to breathe.

for(int i=0;i<limit;i++){  

would be much more readable like

for(int i = 0; i < limit; i++){  

The Scanner is only used for retrieving the values at one place (which should be in main()) so you shouldn't make it a static field. Better declare it inside the method.

\$\endgroup\$
4
\$\begingroup\$

As was noted in another answer there's already a built in method to handle this.

You really need more line breaks in your code, as well as spaces between your operators.

Let's look at your concatArr method as an example.

public static int[] concatArr(int[] arr1,int[] arr2){
    int[] tempArr = new int[arr1.length+arr2.length];
    for(int i=0;i<arr1.length;i++){
        tempArr[i] = arr1[i];
    }
    for(int j=0;j<arr2.length;j++){
        tempArr[j+arr1.length] = arr2[j];
    }
    return tempArr;
}

It's really just one wall of code with everything tightly spaced, in larger projects this affects readability, and becomes error prone. What's interesting is you have areas where you set a variable and see the need for spacing, but this is preceded by doing the same without such consideration.

In general try to:

  • Have a space before and after operators, with the exception of unary operators(++/--)
  • Use line breaks to stratify similar statements/work.
  • Whichever style you choose, be consistent about it.

Applying this to your method it would look more like

public static int[] concatArr(int[] arr1,int[] arr2){
    int[] tempArr = new int[arr1.length + arr2.length];

    for(int i = 0; i < arr1.length; i++) {
        tempArr[i] = arr1[i];
    }

    for(int j = 0; j < arr2.length; j++) {
        tempArr[j + arr1.length] = arr2[j];
    }

    return tempArr;
}

Your initArr method asks the user for a maximum value they want in the array, but the reality is that maximum value isn't possible because calling nextInt on that value makes sure that the integers you get are all non-negative integers below it. If you actually want to include it as I'm sure was intended you need to either initialize maxValue as in.nextInt() + 1 or do rand.nextInt(++maxValue)

\$\endgroup\$
1
  • \$\begingroup\$ As a small note: I would also add spaces in front of { or put the { on a new line. The latter approach helps finding the correct } if you have a complex structure with many nested blocks/loops/if statements etc. \$\endgroup\$
    – BluesSolo
    Feb 9, 2015 at 10:03
2
\$\begingroup\$

I totally agree with all of the above, but will add to it that you are prompting the user for an input of the array size, and then not checking it.

If a negative value is provided for "limit", we have problems!

\$\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.