Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

In this exercise, I'm supposed to sum up elements from 2 arrays, then display the answer on one line with spaces in between. I did it and my answer got accepted as correct, but I'm wondering if there is a better way to do this since I feel my way was more complicated than it needed to be.

import java.util.ArrayList;
import java.util.List;

public class Main {

public static void main(String[] args) {
    int n = 11;

    int[] firstArray = {811594, 993574, 299729,559604,161945,969851,588210,
            692459, 28350,43017,797855};
    int[] secondArray = {725888, 233750,191700,944750,380402,319860,766872,764921,
            330218,906679, 65309};

    List<Object> total = new ArrayList<>();

    for(int i = 0; i < n; i++){
        int sum = firstArray[i] + secondArray[i];
        total.add(sum);
    }

    Integer[] sumArray = total.toArray(new Integer[total.size()]);
    for(int i = 0; i < n; i++) {
        System.out.print(sumArray[i] + " ");
    }


}
}
share|improve this question
    
Yousef, you need to mention, what should happen if both arrays have different sizes? And handle that condition. – Rao Jan 14 at 0:12
    
Rao, the exercise always gives two arrays that are identical in size. It doesn't even give array it gives 2 groups of numbers and asks you to add the numbers from both groups. – Yousef Jan 14 at 0:14
    
If you want to store the intermediate result, you don't need to use a List, you can create an array immediately: int[] total = new int[n];, then total[i] = sum; – njzk2 Jan 14 at 18:53
up vote 11 down vote accepted

You don't need to store the intermediate result. You don't need to give the length of the arrays. This assumes that the arrays are allways of equal length.

for(int i = 0; i < firstArray.length; i++){
    System.out.print((firstArray[i] + secondArray[i]) + " ");
}
share|improve this answer
    
It's important to use .length over a hard coded "magic" number, as you then don't have to update it if your values change, and it becomes very flexible if you want dynamic content in your arrays. – Tom.Bowen89 Jan 14 at 10:52

@Orden is right that you don't need an intermediary array. On top of that, I would point out a few violations of good practices.

Use the right types. If you want to add integers in a list, then use a List<Integer> instead of a List<Object>.

If you know the final size of the list in advance, and you intend to convert it to an array, then consider skipping the list completely, and use just an array from the start. Like this:

int[] sumArray = new int[n];

for(int i = 0; i < n; i++){
    sumArray[i] = firstArray[i] + secondArray[i];
}
share|improve this answer
    
IntelliJ IDEA made me change it to convert it to an array but it was List<Integer> before. – Yousef Jan 14 at 0:20
1  
@Yousef lesson 1 of using an advanced IDE, learn how it works. They don't make you do anything, they provide suggestions that speed up the process. You should not use the advanced functionality of your IDE at all until you have some knowledge of the language. – Boris the Spider Jan 14 at 14:56

The basic idea is right, but I don't like this line and its consequences:

List<Object> total = new ArrayList<>();
  • If you just need to print the result, you don't need to store the sums at all; you can just print as you go.
  • If you want to store the sums, and you know the lengths of the inputs, then you can just make an array instead of an ArrayList to help you do it.
  • If you do use an ArrayList, then providing a capacity estimate would be good practice.
  • List<Object> defeats type safety. List<Integer> would be better.

Also, instead of hard-coding n = 11;, I suggest

assert firstArray.length == secondArray.length;
int n = firstArray.length;

So, one good solution would be

for (int i = 0; i < n; i++) {
    System.out.print((firstArray[i] + secondArray[i]) + " ");
}
System.out.println();     // It's customary to finish with a newline

Another good solution would be

int[] sums = new int[n];
for (int i = 0; i < n; i++) {
    sums[i] = firstArray[i] + secondArray[i];
}
for (int i = 0; i < n; i++) {
    System.out.print(sums[i] + " ");
}
System.out.println();
share|improve this answer

Others have covered bad practices here, I am just going to demonstrate a Java 8 solution to the problem - as Java 8 is the current version of Java; I believe this is what should be written in modern Java:

assert firstArray.length == secondArray.length;
IntStream.range(0, firstArray.length)
    .map(i -> firstArray[i] + secondArray[i])
    .forEach(System.out::println);

This simply creates an IntStream of [0, n) and then uses those indices to map to the sum of the variables.

share|improve this answer
    
That's look so weird from a old java coder point of view but at least I have learnt something new! :-) – Orden Jan 14 at 14:52
    
@Orden certainly very un-Java like. But C# has had lambdas and LINQ for donkey's years and Scala changed the JVM language game somewhat. I find the syntax really readable and I find it enforces SRP as each distinct map operation has it's own, simple, function. – Boris the Spider Jan 14 at 14:54
    
not the best way to use stream I would say. too bad java lacks a zip stream. – njzk2 Jan 14 at 18:54
    
@njzk2 Java 8 decided not to include tuple types so that meant that zip and friends could not be added. I have sorely missed zipWithIndex numerous times in recent months... – Boris the Spider Jan 15 at 8:33

The "modern" java solution puts newlines, not spaces between the elements, of course. Adding the trailing space, and using print instead of println will fix that. Inserting a join (Java has join, right?) might also work.

And, whats' wrong with APL's ⎕←firstArray+secondArray

share|improve this answer
    
It's really hard to understand what you're trying to say. "Inserting a join (Java has join, right?) might also work." sounds like guesswork, and doesn't make a good argument. – janos Jan 14 at 16:16
    
Join may not be the proper term. What I'm thinking of inserts a delimiter between a list of strings, creating one bigger string:/ 'one&two&three' == join('&',['one','two','three']) – Randy A MacDonald Jan 21 at 14:47
    
To be honest, I thought my use of 'might' clearly implied some guesswork.... – Randy A MacDonald Jan 21 at 14:50
    
Your answer as it is doesn't seem to make a clear, good point. I suggest to either rewrite it or delete it. – janos Jan 21 at 15:03
    
Having the elements separated by newlines seems incorrect. The Java solution also seems verbose, to this APL person at least. – Randy A MacDonald Feb 12 at 15:46

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.