There are several major problems with this code, here's a few:
- The threads are created, but never started. This means the code they contain will never run. For that to occur, the
start() method must be called on each thread that is created.
- The threads are not "joined" at the end, so the method will return the value in
c[0] before the threads have had a chance to perform their calculations. For that to occur, the join() method must be called on each thread that is created.
- The updating of
c[0] by the threads is not thread safe. This data, which is concurrently accessed by multiple threads, needs to be protected locking mechanism of some sort.
- On the first iteration of the inner for loop, both
s0 and s1 are 0, on further iterations, s1 is one ahead of s2. Not knowing what is actually wanted by the algorithm, this may or may not be correct, but looks suspicious.
Additionally, there's a few things that are just odd:
- Why declare a single element array of objects
c, for it to only have one element, and never be used as an array, but as simple variable. We probably want to use an AtomicInteger here. That will also solve the concurrent access issue, as AtomicInteger provides thread-safety.
Nothing is typed. For example the array p should be an array of Thread objects:
Thread p[] = new Thread[data.length];
Similarly, the type of c should be Integer[] (or AtomicInteger)
I have annotated the code below:
// Method declaration, takes a two-demensional array of integers
public static int process(int [][] data) {
// Create a one-demensional array of Objects, containing
// a single element, `0`. This will be "auto-boxed" from
// a primitive int to an Integer object.
final Object[] c = new Object[]{0};
// Create a one dimensional array of Objects, of the same
// size as the first dimension of data
Object[] p = new Object[data.length];
// Loop through each index of the array p, declared above
for (int i = 0; i < p.length; i++) {
// get a reference, r, to the ith "row" of data
final int [] r = data[i];
// create a new Thread, and store a reference to it in p[i]
p[i] = new Thread() {
// This is the method that will be invoked on a separate
// thread of execution by the java virtual machine
public void run() {
for (int s0 = 0, s1 = 0; s0 < r.length - 1; s0++, s1 = s0 + 1) {
if (r[s0] > r[s1]) {
// perform an xor-swap of two elements in the
// array (as pointed out by Jongware)
r[s0] ^= r[s1]; r[s1] ^= r[s0]; r[s0] ^= r[s1];
// this access of c[0] is not thread-safe
c[0] = ((Integer) c[0]) + 1;
}
}
}
};
}
// return the value in c[0]
return (Integer) c[0];
}
And here is a version where I've fixed all the things I mention above:
public static int process(int [][] data) {
final AtomicInteger c = new AtomicInteger();
Thread[] threads = new Thread[data.length];
for (int i = 0; i < data.length; i++) {
final int [] row = data[i];
threads[i] = new Thread() {
public void run() {
for (int s0 = 0, s1 = 1; s0 < row.length - 1; s0++, s1 = s0 + 1) {
if (row[s0] > row[s1]) {
row[s0] ^= row[s1]; row[s1] ^= row[s0]; row[s0] ^= row[s1];
c.incrementAndGet();
}
}
}
};
threads[i].start();
}
try {
for (Thread thread : threads)
thread.join();
} catch (InterruptedException e) {}
return c.intValue();
}
