2
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

There is once very significant issue, one significant, and a few run-of-the-mill issues that concern me.

The most significant is the use of Thread.interrupt(). interrupts on threads are not used by anyone, willingly. They are a PITA that few people understand, and everyone hates.

First, though, let me talk about the poison pill. This is a good idea. It is well used. There's an implementation problem though.... Poisong pills are normally one specific instance of an object, and it is shared, typically through a public static field. I would do the pill as:

public static final String QUEUE_POISON_PILL = new String("The end of the queue as we know it");

Note how I create a new String even though the input is a String constant? This is deliberate so I can use reference equality (==) on the check, and guarantee no collisions with some other String. Let's put that pill on the Tester class which is the coordinator for the other classes.

Now, back to the Reverse thread's loop...

The 'industry-standard' way of coding the loop in the reverse thread (using the new poison pill):

    CharSequence seq = null;
    int oddWord = 0;
    while ((seq = intake.take()) != Tester.QUEUE_POISON_PILL) {

        store.put(oddWord % 2 == 1 ? str : reverseWord(str));
        oddWord++;

    }
    store.put(Tester.QUEUE_POISON_PILL)

yes, the while clause condition does an assignment. The alternative is:

    CharSequence seq = intake.take();
    int oddWord = 0;
    while (seq != Tester.QUEUE_POISON_PILL) {

        store.put(oddWord % 2 == 1 ? str : reverseWord(str));
        oddWord++;
        seq = intake.take();

    }
    store.put(Tester.QUEUE_POISON_PILL)
\$\endgroup\$
  • \$\begingroup\$ This makes a lot of sense. Thanks for the input. Maybe it was my use of Thread.currentThread().interrupt() that landed me the low grade. Thanks for the input! I will definitely keep the idea of making the poison pill a specific instance. I did not know you could assign values in a loop, but thanks for the input! \$\endgroup\$ – DanSchneiderNA Oct 28 '14 at 11:27
2
\$\begingroup\$

"Just off a glance", as you requested, I'd say that this is reasonable.

Unless the task description contained additional constraints (e.g. like the constraint that you should have implemented your own blocking queue...), from the first glance, there is nothing really wrong with this implementation.

I personally tend to avoid File objects, and use InputStream/OutputStream instead. This makes programs more flexible, because you can read/write from/to anything that is offered as a stream (e.g. a network connection). An additional benefit in this case would be that they would nicely match the idea of a producer and a consumer.

Concerning the implementation details, there is only one thing that I found really worth noting: The BlockingQueue is static in all your classes. This should NOT be the case. But since you are passing a reference to the queue to these classes in the constructor, I think that this could be solved trivially by removing the static modifier (or even better: replacing it with final).


Off-topic: If this implementation causes trouble (in terms of not passing an assignment, and for the case that none of the aformentioned constraints in the task description applied), I'd strongly recommend to ask for a detailed explaination and request a third party opinion/review

\$\endgroup\$
  • \$\begingroup\$ I passed the assignment but with a mid-C. I asked what I've done wrong and he never gave me anything concrete. I just took it upon myself to see where i could have improved in this program \$\endgroup\$ – DanSchneiderNA Oct 28 '14 at 11:22
  • \$\begingroup\$ I will look at the InputStream/OutputStream as well. It's my first time hearing of them to be honest. Thank you for your input! I appreciate it \$\endgroup\$ – DanSchneiderNA Oct 28 '14 at 11:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.