2 Threads taking turns printing the alphabet

Question

I am supposed to write code for an assignment. The goal is to make 2 threads (which are objects of a class that implements runnable) take turns printing the alphabet. One of them prints upper case while the other one prints lower case. (they print only a single letter each turn, not the whole alphabet, just for clarification)

I feel like my code is pretty self-explainatory but if I am wrong here and you have questions please ask them! I appreciate any help I can get for sure!

The Code:

public class ABCPrinter implements Runnable {

    // --- Attributes ---

   private boolean bool_isUpperCase;
   public static  boolean bool_Switch = true;

   // --- Constructor ---

   public ABCPrinter (boolean init_isUpperCase) {

       this.bool_isUpperCase = init_isUpperCase;

   }

    @Override
    public synchronized void run() { // custom run method

        for (char char_Counter = 'a'; char_Counter <= 'z'; char_Counter++) { // count through the alphabet

            if (bool_isUpperCase){ // decide whether to print upper or lower case

                if(bool_Switch) {

                    System.out.println(Character.toUpperCase(char_Counter));
                    System.out.println("\n----------------------");
                    System.out.println("Message has been sent.");
                    System.out.println("-----------------------");

                    try {

                        Thread.sleep(1000);

                    } catch(Exception e) {

                        System.out.println("\nInterrupted.");

                    }

                    bool_Switch = false;
                    System.out.println("\n--------------------");
                    System.out.println("Switch has been set to false.");
                    System.out.println("-----------------------");

                    try {

                        Thread.sleep(1000);

                        notifyAll();
                        System.out.println("\n--------------------");
                        System.out.println("All threads have been notified.");
                        System.out.println("-----------------------");

                        Thread.sleep(1000);

                        System.out.println("\n--------------------");
                        System.out.println("Thread 1 is entering waiting state.");
                        System.out.println("-----------------------");
                        wait();


                    } catch (Exception e) {

                        System.out.println("Process Interrupted.");

                    }

                } else {

                    try {

                        System.out.println("Thread 1 is waiting.");
                        wait();

                    } catch (Exception e) {

                        System.out.println("Process Interrupted.");

                    }

                }

            } else {

                if(!bool_Switch) {

                    System.out.println(Character.toUpperCase(char_Counter));
                    System.out.println("\n----------------------");
                    System.out.println("Message has been sent.");
                    System.out.println("-----------------------");

                    try {

                        Thread.sleep(1000);

                    } catch(Exception e) {

                        System.out.println("\nInterrupted.");

                    }

                    bool_Switch = true;
                    System.out.println("\n--------------------");
                    System.out.println("Switch has been set to true.");
                    System.out.println("-----------------------");

                    try {

                        Thread.sleep(1000);

                        notifyAll();
                        System.out.println("\n--------------------");
                        System.out.println("All threads have been notified.");
                        System.out.println("-----------------------");

                        Thread.sleep(1000);

                        wait();
                        System.out.println("\n--------------------");
                        System.out.println("Thread 2 is entering waiting state.");
                        System.out.println("-----------------------");

                    } catch (Exception e) {

                        System.out.println("Process Interrupted.");

                    }

                } else {

                    try {

                        System.out.println("Thread 2 is waiting.");
                        wait();

                    } catch (Exception e) {

                        System.out.println("Process Interrupted.");

                    }

                }

            }

        }

    }

}

The main where I execute everything:

public class Main2 {

    public boolean bool_switch;

    public static void main(String[] args){

        ABCPrinter p1 = new ABCPrinter(true);
        ABCPrinter p2 = new ABCPrinter(false);
        
        Thread thr_UpperCase = new Thread(p1);
        Thread thr_LowerCase = new Thread(p2);

        thr_UpperCase.start();
        thr_LowerCase.start();

    }

}

Answer 1

Welcome to code review! :)

Here are a few tips to improve your code:

• Empty lines have a meaning. Do not just spam emtpy lines every where, as that makes the code harder to read. By removing most of the empty lines, I have reduced you code's number of lines to almost have. Empty lines are usually used to separate pieces of code with different concerns.

• DRY (Don't Repeat Yourself).

  1. Your code is filled with the same structure of try-sleep-catch-print. Then, if your actually want to make the thread sleep (which for learning purposes you can do, but usually you wouldn't), then extract that logic into a method.

  2. The logic for thread 1 and thread 2 are exactly the same, changing a few strings. Again, extract the logic to a common method.

• Use consistent casing. In Java, camelCase is usually used to name variables.

• Using a static variable for thread communication is not thread-safe. At the very least, you could use an AtomicBoolean.

• I would separate the thread communication logic from the actual letter printing.

Putting all this together, here is a refactor using queues to synchronise the threads:

public class LetterPrinter {

    private final char end;
    private char currentLetter;

    public LetterPrinter(char start, char end) {
        if (start > end) {
            throw new IllegalArgumentException("Start letter cannot be larger than end letter");
        }
        this.currentLetter = start;
        this.end = end;
    }

    public boolean hasNext() {
        return currentLetter <= end;
    }

    public void printNext() {
        System.out.println(currentLetter);
        currentLetter++;
    }
}

import java.util.concurrent.BlockingQueue;

public abstract class LetterRunnable implements Runnable {
    protected final BlockingQueue<Integer> queue;
    protected final LetterPrinter letterPrinter;

    public LetterRunnable(BlockingQueue<Integer> queue, LetterPrinter letterPrinter) {
        this.queue = queue;
        this.letterPrinter = letterPrinter;
    }

    @Override
    public final void run() {
        while (letterPrinter.hasNext()) {
            try {
                printNextLetter();
            } catch (InterruptedException e) {
                return;
            }
        }
    }

    protected abstract void printNextLetter() throws InterruptedException;
}

import java.util.concurrent.BlockingQueue;

public class UpperCaseRunnable extends LetterRunnable {

    public UpperCaseRunnable(BlockingQueue<Integer> queue) {
        super(queue, new LetterPrinter('A', 'Z'));
    }

    @Override
    protected void printNextLetter() throws InterruptedException {
        letterPrinter.printNext();
        queue.put(0); // notify
        queue.put(0); // wait
    }
}

import java.util.concurrent.BlockingQueue;

public class LowerCaseRunnable extends LetterRunnable {

    public LowerCaseRunnable(BlockingQueue<Integer> queue) {
        super(queue, new LetterPrinter('a', 'z'));
    }

    @Override
    protected void printNextLetter() throws InterruptedException {
        queue.take(); // wait
        letterPrinter.printNext();
        queue.take(); // notify
    }
}

import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;

public class Main {
    public static void main(String[] args) throws InterruptedException {

        BlockingQueue<Integer> queue = new ArrayBlockingQueue<>(1);

        UpperCaseRunnable upperCaseRunnable = new UpperCaseRunnable(queue);
        LowerCaseRunnable lowerCaseRunnable = new LowerCaseRunnable(queue);

        Thread threadUpperCase = new Thread(upperCaseRunnable);
        Thread threadLowerCase = new Thread(lowerCaseRunnable);

        threadUpperCase.start();
        threadLowerCase.start();

        threadUpperCase.join();
        threadLowerCase.join();
    }
}

Answer 2

My word! People seem to go for complexity here, don't they? (and verbosity).

This problem should be simple enough. I won't write any code, as it's someone else's homework, but here's my thinking.

You need two threads. Each needs to know two things:

1. Am I outputting upper or lower case?
2. What is the other thread?

The main loop of each is this

• wait()
• when notified, output the next letter in the appropriate case
• notify the other thread
• If we haven't run out of letters, return to the initial wait()
• When we have run out, end the thread by leaving the run() method

The main method simply needs to create these threads, start() one of them and join() to both.

This is somewhat crude, and I have a strong suspicion that there's a potential timing window in the case where a freshly woken thread attempts to wake the other before the other reaches the wait(). This could be dealt with by adding an "allowed to proceed" boolean to each Runnable, which is checked and cleared by the thread itself, but set (in a synchronized block) by the other thread before notifying it.

I can't see value in using a queue for synchronization. Perhaps I missed Miguel's point somehow. If you wanted to use java.util.concurrency functionality, probably something like a Phaser would be a reasonable choice, but basic wait/notify seems sufficient.