I found this Java code in a job interview question and tried to figure it out. (I am a Smalltalker and am less familar with the syntax of Java as maybe I thought).
As far as I can see, it spawns a thread for every row in a data matrix. The thread seems to process each column in the row-vector pairwise. It checks if the first item in the pair is bigger then the second. If true it increments a global counter(?) and does something to the data, that I don't understand (What's "or" defined to do with integers in Java?).
I believe it is somehow broken, too. Can you help me figure it out? I want to understand what's going on.
public static int process(int [][] data) {
final Object[] c = new Object[]{0};
Object[] p = new Object[data.length];
for (int i = 0; i < p.length; i++) {
final int [] r = data[i];
p[i] = new Thread() {
public void run() {
for (int s0 = 0, s1 = 0; s0 < r.length - 1; s0++, s1 = s0 + 1) {
if (r[s0] > r[s1]) {
// what is this next line supposed to do?
r[s0] ^= r[s1]; r[s1] ^= r[s0]; r[s0] ^= r[s1];
// does this next line increase a global counter?
c[0] = ((Integer) c[0]) + 1;
}
}
}
};
}
return (Integer) c[0];
}
There are several major problems with this code, here's a few:
start()
method must be called on each thread that is created.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.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.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:
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();
}