Initialize and sum an array in Java using threads

This bit of code initializes an array of any given size where each index corresponds to a thread and a value. That is, index 0 has 0, index 1 has 1, and so on. Then, the values, 0 to n, in the indexes are summed.

This code as part of an assignment though I'm wondering if there are any ways it could be improved? Namely, is there an alternative to using 2 for loops and calling thread.join() on them?

I'd be open to any advice and would gladly take it!

public class Processor {

    static volatile int numberOfValues = 17;
    static double processedArray[];
    static Thread threadsArray[];
    static volatile int sum;
    static Object lock1 = new Object();
    static Object lock2 = new Object();

Adding values to the array, each index in the array gets a thread to initialize that value:

private static void initializeArray() {
        threadsArray = new Thread[numberOfValues];
        processedArray = new double[numberOfValues];

        for (int i = 0; i < threadsArray.length; i++) {
            threadsArray[i] = new Thread(new Runnable() {
                public void run() {
                    synchronize(lock1) {
                        processedArray[numberOfValues - 1] = numberOfValues;
                        numberOfValues--;
                    }

                }   
            });
            threadsArray[i].start();
        }

Here is the first for loop joining all threads after the initialization of the array:

for (int i = 0; i < threadsArray.length; i++) {
            try {
                threadsArray[i].join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

Summing the values in the array:

```
for (int i = 0; i < threadsArray.length; i++) {
threadsArray[i] = new Thread(new Runnable() {
public void run() {
synchronize(lock2) {
sum += processedArray[numberOfValues];
n

In this answer I want to take a look at what your code is doing, and why it doesn't really make sense to use threads like this.

One of the main point of using threads is to do long-running things at the same time (more details here). Some examples:

• keeping a GUI responsive while doing other work in the background.

• Spreading long calculations over multiple cores.

• Doing long-running tasks like waiting for incoming connections or collecting statistics.

If you do them on the same thread, they have to wait for each other. Keep each one on a separate thread, and they'll happily run side-by-side.

In your code, this doesn't really happen.

for (int i = 0; i < threadsArray.length; i++) {
        threadsArray[i] = new Thread(new Runnable() {
            public void run() {
                synchronize(lock1) {
                    processedArray[numberOfValues - 1] = numberOfValues;
                    numberOfValues--;
                }

            }   
        });
        threadsArray[i].start();
    }

Here you're creating your first set of threads, to set the values of the array. However, all of the code inside the thread is synchronized over one object.

static Object lock1 = new Object();

Only one thread can hold a lock at a time. So while one thread is doing its thing, the others are just waiting. This means that all the threads you've created are waiting for each other. They're not running at the same time at all!

But think about why threads are useful: they allows you to do long running tasks at the same time. Except these threads aren't doing anything long. They're just setting a value in an array. That's super fast. It probably takes longer to make the thread than to set the value.

So the same code could have been written like this, without threads:

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

And it would have been more readable, shorter and faster.

Addendum

As I've said before, this problem should not be solved using threads. However as an exersize let's look at why you need a lock, and how to get rid of it.

public void run() {
    synchronize(lock1) {
        processedArray[numberOfValues - 1] = numberOfValues;
        numberOfValues--;
    }
}

You're sharing the variable numberOfValues between threads. This means that without lock:

• One thread could excecute the first line.

• Another thread could execute the same line.

Because numberOfValues hasn't been decremented, both proccessArray[] values are now set to the same value.

Instead of using a normal integer as a counter you could use an AtomicInteger.

static AtomicInteger numberOfValues = new AtomicInteger(17);

Your code will look like this:

public void run() {
    int index = numberOfValues.decrementAndGet();
    processedArray[index] = index;
}

You won't need a lock now, since all access to the AtomicInteger is atomic. There's no way to set a proccessArray value without also decrementing the counter.

But we can do better. Instead of using a counter, why don't we just use the index of the loop you're using to create the threads? The code will look like this:

for (int i = 0; i < threadsArray.length; i++) {
    final int index = i;
    threadsArray[i] = new Thread(new Runnable() {
        @Override
        public void run() {
            processedArray[index] = index;
        }
    });
}

Now you don't need a shared counter, and you wont't need a lock.

StackExchange Code Review Q#123305, answer score: 3