I agree with @Loki Astari that this is overkill but as a homework for a course is fine. Some notes about the current code:
1.
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.
1.
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 Executor
s and Future
s 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<Future<?>> initalizerFutures = new ArrayList<>();
w = new int[size];
for (int i = 0; i < size; i++) {
// runs the Initializer with the executor on separate threads
final Future<?> 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.
1.
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
andw
. 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)
1.
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?)
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?
- 30.3k
- 9
- 82
- 157