Skip to main content
Code Review

Return to Revisions

2 of 5
added 871 characters in body
palacsint
  • 30.3k
  • 9
  • 82
  • 157

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 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<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.)

  1. 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.

  1. 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).

  1. 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.

  2. 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?)

  1. 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.

  1. 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?

palacsint
  • 30.3k
  • 9
  • 82
  • 157
default

AltStyle によって変換されたページ (->オリジナル) /