Using threads to find max in array

I really do not have access to anyone, outside of professors, that is highly knowledgeable about Java so I figured I would post my stuff here to further my knowledge and improve my coding. This is homework for my Operating Systems class and it works. Here are some of my main concerns.

• Is this a good use of the static inner class?

• Are my variable and class declarations properly declared?

• Is there a better way than using Thread.sleep to make the sure that the main thread waits for certain child threads to complete before it performs some action?

The object of the program is to find the max in an array using 3 steps. Each step is to perform one part of the task with its own thread. For example, step one uses n threads to initialize an array of size n with 1's. Input done via command line: Prog1.java n x0 x1 ... xn, where n is the number of elements and x0 x1 ... xn are the elements. Input is limited to about 20 elements.

As I am a student, I will also glady accept any critiques on any part of the code. Sorry if this seems like too much for one question and thanks in advance to anyone who contributes.

```
/* User Input: 4 3 1 7 5
Number of input values = 4
Input values x = 3 1 7 5
After initialization w = 1 1 1 1
Thread T(0,1) compares x[0] = 3 and x[1] = 1, and writes 0 into w[1]
Thread T(2,3) compares x[2] = 7 and x[3] = 5, and writes 0 into w[3]
Thread T(1,3) compares x[1] = 1 and x[3] = 5, and writes 0 into w[1]
Thread T(1,2) compares x[1] = 1 and x[2] = 7, and writes 0 into w[1]
Thread T(0,3) compares x[0] = 3 and x[3] = 5, and writes 0 into w[0]
Thread T(0,2) compares x[0] = 3 and x[2] = 7, and writes 0 into w[0]
After Step 2 w = 0 0 1 0
Maximum = 7
Location = 2
*/

import java.util.Arrays;
public class Prog1 {
private static int[] w;
private static int[] x;

public static void main(String[] args) throws InterruptedException {
if (args.length == 0) { /*

I agree with @Loki Astari that this is overkill but as a homework for a course is fine. Some notes about the current code:

-

Is this a good use of the static inner class?

It's OK but I prefer putting them to separate files. I've found that switching between windows/tabs is more handy than scrolling in the same file. You can do it if you pass the w and/or x arrays to their constructor.

-

Is there a better way than using Thread.sleep to make the sure that the main thread waits for certain child threads to complete before it performs some action?

Yes. On low level Thread.join but I suggest you using higher level structures, like Executors and Futures for that.

I'd implements Runnable than extends Thread and change the following loop

for (int i = 0; i < size; i++) {
    Initialize t = new Initialize(i);
    t.start();
}

to

// creates an executor with size threads
ExecutorService executorService = Executors.newFixedThreadPool(size);
List> initalizerFutures = new ArrayList<>();
w = new int[size];
for (int i = 0; i  initalizerFuture = executorService.submit(new Initialize(i));
    initalizerFutures.add(initalizerFuture);
}
// waits for each initializer to complete
for (final Future future: initalizerFutures) {
    future.get();
}

(At the end of the program you should shutdown the executor.)

-
Unfortunately, the code is still not thread safe. Although accessing array elements on separate threads could be safe but the code in the question accesses the same array index from separate threads which should be synchronized properly.


[...] synchronization has no effect unless both read and write operations are synchronized.

Source: Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data


Locking is not just about mutual exclusion; it is also about memory visibility.
To ensure that all threads see the most up-to-date values of shared mutable
variables, the reading and writing threads must synchronize on a common lock.

Source: Java Concurrency in Practice, 3.1.3. Locking and Visibility.

-

Arrays.toString(a).replace("[", "").replace("]", "").replace(",", "");

This is duplicated in the code. You should move this logic into a separate method and use that to keep it DRY. Additionally, I guess the format of Arrays.toString won't change but it would be cleaner iterating through the array and building the string with a loop or using an already existing join implementation.

-
In System.out.printf use %n instead of \n. The former outputs the correct platform-specific line separator.

-

System.out.printf("%-21s%s\n", "Input values ", "x = " + input);

could be

System.out.printf("%-21s%s%s%n", "Input values ", "x = ", input);

(without the concatenation).

-
Short variable names are hard to read, like x and w. Longer names would make the code more readable since readers don't have to decode the abbreviations every time and when they write/maintain the code don't have to guess which abbreviation the author uses.

-
Comments like this are rather noise:

if (args.length == 0) { /* return if there are NO arguments */

It's rather obvious from the code itself, so I'd remove the comment. Your professors might require these comments but keep in mind that it's not clean. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

-

System.out.println("No arguments!");

Error messages shouldn't blame the user (don't say what he did is wrong), give a suggestion about what they should do. (See: Should we avoid negative words when writing error messages?, What will be the Best notifications and error messages?)

-
The Java Language Specification, Java SE 7 Edition, 6.1 Declarations:


Class and Interface Type Names


Names of class types should be descriptive nouns or noun phrases, not overly long, in mixed
case with the first letter of each word capitalized.

According to that I'd rename Initialize to Initializer, FindMax to MaximumFinder, PrintMax to MaximumPrinter.

-
Comments on the closing curly braces are unnecessary and disturbing. Modern IDEs could highlight blocks.

} // End main()

“// …” comments at end of code block after } - good or bad?

StackExchange Code Review Q#43834, answer score: 5