Search code examples
javamatrixadjacency-matrix

Can you help me figuring out, what this Snipped of Java-code is supposed to do?


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

Solution

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