current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 7 down vote favorite

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;
}
share|improve this question
1  
I don't think there's a whole lot to say... Looks about as simple as it can get, IMO –  Phrancis yesterday
    
Haha, thanks! I think I'll try a different problem then, and close this as an answer myself if there's no other feedback. –  cody.codes yesterday
1  
@Phrancis there's always something to say ;-) –  Mat's Mug yesterday

4 Answers 4

up vote 12 down vote accepted

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;
}
share|improve this answer
    
ah, I like first and second –  Mat's Mug yesterday
3  
@Mat'sMug It's surely less confusing that list1 and list2. My very first thought was "Arrays.copyOf and lists don't fit together". –  maaartinus yesterday
    
Love the fact that you're able to consolidate the code so much. –  cody.codes yesterday
1  
This implementation is reputedly faster because System.arraycopy() is implemented natively. –  200_success♦ 23 hours ago
up vote 6 down vote

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;
}
share|improve this answer
up vote 5 down vote

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;
}
share|improve this answer
2  
A "merge" is something clever, like in merge sort. I'd call it simply "concat". +++ Btw., I always use "result". –  maaartinus yesterday
    
@maaartinus eh, concat would be ideal. er, no, IEnumerable<T>.Contat would be! ;-) –  Mat's Mug yesterday
1  
You surely mean java.util.arrays.Concat, right? Just no C# manners. :D:D:D –  maaartinus yesterday
    
@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 –  Mat's Mug yesterday
    
OK, this corresponds with Iterable<T>, except that arrays are special in Java. Unfortunately, Java came first. –  maaartinus yesterday
up vote 1 down vote

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

}
share|improve this answer

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.