I am a semi-new Java programmer that tends to be a perfectionist. What I'd like to know about my code:
- How it compares to common practice.
- Is it readable?
- Are my comments appropriate?
- Any improvements that could be made?
I'm not asking to bog down your time to do an in depth review, just off a glance. I received a poor score from a professor with no explanation other than it "Not being suitable", though it follows all directions posted.
My program:
This program takes a word from a file specified by a user and puts it in a location specified by a user. It takes a single word from the file at a time and places it in a blocking queue. It then goes to a different thread that will reverse every other word starting with the second word (not including punctuation). This thread places it into a different blocking queue for the last thread to write to the file specified.
Main:
import java.io.File; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.BlockingQueue; import java.util.concurrent.Executors; import java.util.concurrent.ExecutorService; import javax.swing.JFileChooser; /** * Test the producer consumer package * * 25/10/2014 * * @author Tyler Weaver */ public class Tester { public static void main(String[] args) { final int MAX_SIZE = 2; //Max queue size //Declarations File input, output; JFileChooser chooser = new JFileChooser(); BlockingQueue<CharSequence> fromReader = new ArrayBlockingQueue(MAX_SIZE); BlockingQueue<CharSequence> toWriter = new ArrayBlockingQueue(MAX_SIZE); ExecutorService service = Executors.newCachedThreadPool(); int returnVal, exitVal; //Declarations //Retrieves the input file do { input = null; returnVal = chooser.showOpenDialog(null); if (returnVal == JFileChooser.APPROVE_OPTION) { input = chooser.getSelectedFile(); } } while (returnVal != JFileChooser.APPROVE_OPTION); //Retrieves the input //Retrieves the output file do { output = null; exitVal = chooser.showSaveDialog(null); if (exitVal == JFileChooser.APPROVE_OPTION) { output = chooser.getSelectedFile(); } } while (exitVal != JFileChooser.APPROVE_OPTION); //Retrieves the output file //Declare Runnable objects Runnable reader = new WordReader(input, fromReader); Runnable rev = new WordReverser(fromReader, toWriter); Runnable writer = new WordWriter(output, toWriter); //Execute threads and shutdown service.execute(reader); service.execute(rev); service.execute(writer); service.shutdown(); } }
WordReader: The thread that reads the words from a file one at a time.
package ProducerConsumerAssignment; import java.io.File; import java.io.FileNotFoundException; import java.io.FileReader; import java.util.concurrent.BlockingQueue; import java.util.Scanner; /** * Reads words from a file and places them into a blocking queue to be read * * 25/10/2014 * * @author Tyler Weaver */ public class WordReader implements Runnable { //The blocking queue to store private static BlockingQueue<CharSequence> bin; private final File loc; //File to read from private final String END_FLAG = "Terminate the queue"; //^Pill for the Poison Pill Technique. Works because every String sent //through the queue will always be one word. Because this is multiple words //There is no way another string could have the same value. /** * Constructor for WordReader * * @param input the text file to read from * @param bin the blocking queue to store the words */ public WordReader(final File input, BlockingQueue bin) { loc = input; WordReader.bin = bin; } /** * Called when being executed Reads words from a file and places into a * blocking queue */ @Override public void run() { try (Scanner in = new Scanner(new FileReader(loc))) { while (in.hasNext()) { bin.put(in.next()); } //Once no more words in the file send the poison bin.put(END_FLAG); } catch (FileNotFoundException ex) { System.err.printf("Error finding File!%n%s%n", ex); } catch (InterruptedException ex) { System.err.printf("File Reader thread was interrupted!%n%s%n", ex); } } }
Word Reverser: The thread that reverses every other word starting with the second word.
package ProducerConsumerAssignment; import java.util.concurrent.BlockingQueue; /** * Takes a word from a blocking queue and reverses it. Puts the reversed word * into another blocking queue. * * 25/10/2014 * * @author Tyler Weaver */ public class WordReverser implements Runnable { private final String END_FLAG = "Terminate the queue"; //^Pill for the Poison Pill Technique. Works because every String sent //through the queue will always be one word. Because this is multiple words //There is no way another string could have the same value. private static BlockingQueue<CharSequence> intake, store; private int oddWord; //Counter for odd words /** * Constructor for Word Reverser * * @param intake the blocking queue to retrieve words from * @param store the blocking queue to store the words */ public WordReverser(BlockingQueue intake, BlockingQueue store) { WordReverser.intake = intake; WordReverser.store = store; oddWord = 0; } /** * Called when being executed. Reverses a word by taking from intake and * places the reversed word into store */ @Override public void run() { boolean isInterrupted = false; while (!isInterrupted) { try { StringBuilder str = new StringBuilder(intake.take()); //Exit condition if (str.toString().equalsIgnoreCase(END_FLAG)) { Thread.currentThread().interrupt(); } //If it is a word to be reversed, then reverse it if (oddWord % 2 == 1) { str = reverseWord(str); } //Put word in queue and increment counter store.put(str); ++oddWord; } catch (InterruptedException ex) { isInterrupted = true; } } //Puts pill into queue when main body is done try { store.put(END_FLAG); } catch (InterruptedException ex) { System.err.printf("Error setting flag in store.%nWordReverser%n%s%n", ex); } } /** * Reverses a word, leaving behind punctuation if there is any * * @param word the word to reverse * @return a string builder object containing the reversed word */ private StringBuilder reverseWord(StringBuilder word) { char punct = Character.MAX_VALUE; //If has punctuation at the end, remove the punctuation if (!Character.isLetterOrDigit(word.charAt(word.length() - 1))) { punct = word.charAt(word.length() - 1); word.deleteCharAt(word.length() - 1); } word = word.reverse(); if (punct == Character.MAX_VALUE) { return word; } return word.append(punct); } }
Lastly is the WordWriter: It writes a single word at a time to a file.
package ProducerConsumerAssignment; import java.io.BufferedWriter; import java.io.File; import java.io.FileWriter; import java.io.IOException; import java.util.concurrent.BlockingQueue; /** * Takes a reversed word from the queue and writes it to a file * * 25/10/2014 * * @author Tyler Weaver */ public class WordWriter implements Runnable { private final String END_FLAG = "Terminate the queue"; //^Pill for the Poison Pill Technique. Works because every String sent //through the queue will always be one word. Because this is multiple words //There is no way another string could have the same value. private static BlockingQueue<CharSequence> in; private final File output; /** * Constructs a WordWriter object * * @param file the file to write words to * @param queue the blocking queue to retrieve words from */ public WordWriter(final File file, BlockingQueue queue) { output = file; in = queue; } /** * Executes when being called in a thread */ @Override public void run() { boolean isInterrupted = false; try (BufferedWriter writer = new BufferedWriter(new FileWriter(output))) { //Continue writing until the thread is interrupted while (!isInterrupted) { CharSequence word = in.take(); if (word.toString().equalsIgnoreCase(END_FLAG)) { isInterrupted = true; } else { writer.write(word + " "); } } } catch (InterruptedException | IOException ex) { System.err.printf("Error writing to output file!%n%s%n", ex); } } }
Any opinions on how to improve my coding would be greatly appreciated. I am always eager to learn.
