7
\$\begingroup\$

This is a simple piece of code which takes two arrays supplied as parameters, then appends them to a new array that contains the values from the supplied arrays. Any suggestions for improvement would be greatly appreciated.

public static int[] append(int[] list1, int[] list2) {
    int[] appendedList = new int[list1.length + list2.length];
    for(int i = 0; i < list1.length; i++) {
        appendedList[i] = list1[i];
    }
    for(int i = 0; i < list2.length; i++) {
        appendedList[i + list1.length] = list2[i];
    }
    return appendedList;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think there's a whole lot to say... Looks about as simple as it can get, IMO \$\endgroup\$ – Phrancis Jun 23 '15 at 1:45
  • \$\begingroup\$ Haha, thanks! I think I'll try a different problem then, and close this as an answer myself if there's no other feedback. \$\endgroup\$ – cody.codes Jun 23 '15 at 1:47
  • 1
    \$\begingroup\$ @Phrancis there's always something to say ;-) \$\endgroup\$ – Mathieu Guindon Jun 23 '15 at 2:02
12
\$\begingroup\$

You could offload some of the work to Arrays.copyOf and System.arraycopy:

public static int[] append(int[] first, int[] second) {
  int[] result = Arrays.copyOf(first, first.length + second.length);
  System.arraycopy(second, 0, result, first.length, second.length);
  return result;
}
\$\endgroup\$
  • \$\begingroup\$ ah, I like first and second \$\endgroup\$ – Mathieu Guindon Jun 23 '15 at 2:00
  • 3
    \$\begingroup\$ @Mat'sMug It's surely less confusing that list1 and list2. My very first thought was "Arrays.copyOf and lists don't fit together". \$\endgroup\$ – maaartinus Jun 23 '15 at 2:28
  • \$\begingroup\$ Love the fact that you're able to consolidate the code so much. \$\endgroup\$ – cody.codes Jun 23 '15 at 4:23
  • 1
    \$\begingroup\$ This implementation is reputedly faster because System.arraycopy() is implemented natively. \$\endgroup\$ – 200_success Jun 23 '15 at 7:01
6
\$\begingroup\$

The code obviously works, and it's simple enough to read. I guess IMO what is missing is context. Consider adding JavaDoc, perhaps?

Example:

/**
 * The purpose of this class is to append sales from multiple stores in one array.
 * @param list1 An array of sales from store # 1
 * @param list2 An array of sales from store # 2
 * @TODO Rename list1 and list2 to more relevant names
 */
public static int[] append(int[] list1, int[] list2) {
    int[] appendedList = new int[list1.length + list2.length];
    for(int i = 0; i < list1.length; i++) {
        appendedList[i] = list1[i];
    }
    for(int i = 0; i < list2.length; i++) {
        appendedList[i + list1.length] = list2[i];
    }
    return appendedList;
}
\$\endgroup\$
5
\$\begingroup\$

list seems like a misleading name/suffix to use - you're using arrays, not lists.

append is also a bit misleading, since you're really merging the inputs into a new output - and the function could say what is being merged, too. I'm thinking mergeArrays.

Other than that, maybe I'd just use result instead of appendedList, but that's just me using result for return values - maybe appendedList is a better, more meaningful naming.

You're accessing list1.length and list2.length a number of times... might want to extract two little locals there, and readability-wise, I'd probably add a bit of extra vertical whitespace, too:

public static int[] mergeArrays(int[] array1, int[] array2) {

    int length1 = array1.length
    int length2 = array2.length
    int[] result = new int[length1 + length2];

    for(int i = 0; i < length1; i++) {
        result[i] = array1[i];
    }
    for(int i = 0; i < length2; i++) {
        result[i + length1] = array2[i];
    }

    return result;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ A "merge" is something clever, like in merge sort. I'd call it simply "concat". +++ Btw., I always use "result". \$\endgroup\$ – maaartinus Jun 23 '15 at 2:31
  • \$\begingroup\$ @maaartinus eh, concat would be ideal. er, no, IEnumerable<T>.Contat would be! ;-) \$\endgroup\$ – Mathieu Guindon Jun 23 '15 at 2:33
  • 1
    \$\begingroup\$ You surely mean java.util.arrays.Concat, right? Just no C# manners. :D:D:D \$\endgroup\$ – maaartinus Jun 23 '15 at 2:37
  • \$\begingroup\$ @maaartinus yeah yeah.... except IEnumerable<T> covers anything from an array to a list, dictionary, hashset or anything enumerable that's not even yet written :p \$\endgroup\$ – Mathieu Guindon Jun 23 '15 at 2:40
  • \$\begingroup\$ OK, this corresponds with Iterable<T>, except that arrays are special in Java. Unfortunately, Java came first. \$\endgroup\$ – maaartinus Jun 23 '15 at 2:51
1
\$\begingroup\$

Using Java 8 you can offload all the work to streams:

public class IntArrayConcatenator {

    /**
     * Concatenates two int arrays
     */
    public static int[] concat(int[] first, int[] second) {
        return IntStream.concat(Arrays.stream(first), Arrays.stream(second)).toArray();
    }

    /**
     * Concatenates any number of int arrays
     */
    public static int[] concat(int[]... arrays) {
        return Arrays.stream(arrays).reduce(new int[0], IntArrayConcatenator::concat);
    }

}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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