0
\$\begingroup\$

I have implemented a simple example for inter thread communication which have 2 threads to print even and odd numbers in sequence.

I am looking for a feedback and points for improving the example.

My code looks like:

public class InterThreadCommunicationEvenOddExample
{
    public static void main(String[] args)
    {
        NumberResource numberResource = new NumberResource();
        new EvenThread(numberResource);
        new OddThread(numberResource);
    }
}


class EvenThread implements Runnable
{

    NumberResource numberResource;



    EvenThread(final NumberResource numberResource)
    {
        this.numberResource = numberResource;
        new Thread(this, "Even").start();
    }



    public void run()
    {
        while (true)
        {
            System.out.println("Even Thread::" + this.numberResource.getNextEven());
            try
            {
                Thread.sleep(1000);
            }
            catch (InterruptedException e)
            {

            }
        }
    }

}


class OddThread implements Runnable
{

    NumberResource numberResource;



    OddThread(final NumberResource numberResource)
    {
        this.numberResource = numberResource;
        new Thread(this, "Odd").start();
    }



    public void run()
    {
        while (true)
        {
            System.out.println("Odd Thread::" + this.numberResource.getNextOdd());
            try
            {
                Thread.sleep(1000);
            }
            catch (InterruptedException e)
            {

            }
        }
    }
}


class NumberResource
{

    private int number = 0;



    public synchronized int getNextEven()
    {
        while (isEven(number))
        {
            try
            {
                wait();
            }
            catch (InterruptedException e)
            {

            }

        }
        number++;
        notify();
        return number;
    }



    public synchronized int getNextOdd()
    {
        while (!isEven(number))
        {
            try
            {
                wait();
            }
            catch (InterruptedException e)
            {

            }

        }
        number++;
        notify();
        return number;
    }



    private boolean isEven(int number)
    {
        return number % 2 == 0;
    }

}
\$\endgroup\$
1
\$\begingroup\$

There is some refactoring that could be made.

Naming

EvenThread and OddThread aren't really threads since they don't implement the Thread class. There are two options:

  1. Rename the two classes to something more appropriate/fitting with its purpose
    • e.g., EvenRunner/OddRunner
  2. Move the thread management to the InterThreadCommunicationEvenOddExample.main method. This is more standard.

Redundancy

  • OddThread and EvenThread have almost the same functionality; a NumberThread could be created that paramaterizes the parity (e.g., a Parity enum parameter). This would be ideal since the logic in run() would only need to be changed once if needed.
  • The above point could be applied to getNextOdd and getNextEven in a similar manner.

Modifiers

Some variables could use tighter modifiers to better convey their purpose.

  • NumberResource numberResource could be final in all of its occurrences.
  • private boolean isEven(int number) could be made static since it is constant with respect to each NumberResource instance; furthermore, a semantic argument could be made that isEven(number) is not asking whether the NumberResource instance is even but rather asking if the provided number is even.

Threads

  • There is no need to call Thread.sleep() inside EvenThread.run() or OddThread.run(), since there is a wait() and notify() statement in the NumberResource's block. A single thread at a time will "wake up" from the wait() call after notify() is called in another thread, so removing the Thread.sleep() calls won't burn CPU cycles.
  • If the Thread.sleep() call is there for the sole purpose of text not flashing quickly onto the screen, then that is fine. Though moving the Thread.sleep() into the NumberResource method(s), before the notify() call, would cause text to be printed more evenly.

Empty catch statements

  • Either log something here or write a comment explaining why no action is taken here.
  • Allowing getNextOdd/getNextEven to throw an InterruptedException exception might even be cleaner.
  • A javadoc string for each method/class could clarify this as well.

Spacing

Overall, the spacing is fine and consistent, but 3 empty lines between each class method seems like too much, but this is subjective.


It is interesting to note that the overall output order of the messages is not guaranteed since notify() is called inside of getNextOdd/getNextEven; however, it is guaranteed that each Even Thread::{num} message will be strictly increasing (by 2). The same goes for each Odd Thread::{num} message. Below is an example of some of the refactoring comments made above (except adding Javadocs).

public class InterThreadCommunicationEvenOddExample {
    public static void main(String[] args) throws InterruptedException {
        final NumberResource numberResource = new NumberResource();
        final Thread[] numberThreads = new Thread[] {
            new Thread(new NumberRunner(numberResource, Parity.EVEN)),
            new Thread(new NumberRunner(numberResource, Parity.ODD)),
        };

        for (Thread numberThread : numberThreads) {
            numberThread.start();
        }

        for (Thread numberThread : numberThreads) {
            numberThread.join();
        }
    }
}

class NumberResource {
    private int number = 0;

    public synchronized int getNextNumber(Parity parity) throws InterruptedException {
        while (getParity(number) != parity) {
            wait();
        }
        final int nextNumber = number++;
        notify();
        return nextNumber;
    }

    private static Parity getParity(final int number) {
        return number % 2 == 0 ? Parity.EVEN : Parity.ODD;
    }
}

class NumberRunner implements Runnable {
    private final NumberResource numberResource;
    private final Parity parity;

    NumberRunner(final NumberResource numberResource, final Parity parity) {
        this.numberResource = numberResource;
        this.parity = parity;
    }

    @Override
    public void run() {
        while (true) {
            try {
                System.out.printf("%s Thread::%d\n", parity.name(), numberResource.getNextNumber(parity));
            } catch (InterruptedException e) {
                System.out.println("Interrupted while trying to retrieve next number.");
                return;
            }
        }
    }
}

enum Parity {
    EVEN,
    ODD,
}
\$\endgroup\$

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.