Printing even and odd using two concurrent threads

Question

Please review the code below where thread Odd prints all odd numbers from 1 to 9 and thread Even prints all even numbers from 2 to 8. I tested it and it works but can we improve it from design or performance perspective?

To print odd

public class Odd implements Runnable{

private Monitor sharedObject;

public Odd(Monitor monitor){
    this.sharedObject = monitor;
}


@Override
public void run() {

    try {
        printOdd();
    } catch (InterruptedException e) {          
        e.printStackTrace();
    }
}


private void printOdd() throws InterruptedException {
    synchronized(sharedObject){
        for(int i=1; i< 10; i++){
            if(i%2!=0){
                while(!sharedObject.isOdd()){
                    sharedObject.wait();
                }
                System.out.println("Odd: "+ i);
                sharedObject.setOdd(false);
                sharedObject.notifyAll();
            }
        }
    }

}

To print even

public class Even  implements Runnable{

private Monitor sharedObject;

public Even(Monitor monitor){
    this.sharedObject = monitor;
}


@Override
public void run() {

    try {
        printEven();
    } catch (InterruptedException e) {          
        e.printStackTrace();
    }
}


private void printEven() throws InterruptedException {
    synchronized(sharedObject){         
        for(int i=1; i< 10; i++){
            if(i%2==0){
                while(sharedObject.isOdd()){
                    sharedObject.wait();
                }
                System.out.println("Even: "+ i);
                sharedObject.setOdd(true);
                sharedObject.notifyAll();
            }
        }
    }

}

Monitor class

public class Monitor {

    private volatile boolean isOdd;

    public boolean isOdd() {
      return isOdd;
    }

    public void setOdd(boolean isOdd) {
      this.isOdd = isOdd;
    }

}

Answer 1

public class Odd implements Runnable{

private Monitor sharedObject;

public Odd(Monitor monitor){
    this.sharedObject = monitor;
}

You implemented the class as a Runnable - great first step. You are injecting the shared Monitor in the constructor, which is good; the member variable should be final though -- you aren't intending to change it after initialization. Also, it should be named "monitor".

@Override
public void run() {

    try {
        printOdd();
    } catch (InterruptedException e) {          
        e.printStackTrace();
    }
}

If you are trying to learn about using multiple threads, one of the things you must come to understand is what InterupptedException means. In particular, printStackTrace isn't likely to tell you anything interesting (yup, the exception was thrown by sharedObject.await()), and if you are going to catch the exception, you must reset the interrupt flag so that the function that called you can know that cancellation is in progress.

private void printOdd() throws InterruptedException {
    synchronized(sharedObject){
        for(int i=1; i< 10; i++){
            if(i%2!=0){
                while(!sharedObject.isOdd()){
                    sharedObject.wait();
                }
                System.out.println("Odd: "+ i);
                sharedObject.setOdd(false);
                sharedObject.notifyAll();
            }
        }
    }
}

You are testing your condition in a while() loop -- good job, that's really important. You are also notifying all threads that are blocked on the sharedObject, which is another Good Thing.

There's a generalization to your code that might be appropriate to make explicit:

for(int i=1; i< 10; i++){
    if(i%2!=0){
        ...
    }
}

The for loop is... well, here's the thing. The threads are supposed to be taking turns counting the same numbers, right? That is more clearly expressed in the code by having the two threads share a data structure

for(int i : this.numbersToCount){

Where possible, you should use functions to clarify the intent of complex boolean expressions:

if( isOdd(i))

The big problem I see is that you then duplicate all of your code to create Even. Something has gone very badly wrong. Let's generalize your loop, by replacing the thread mechanics with functions that express your intent.

synchronized(sharedObject){
    while(this.notFinished()) {
        this.waitForMyTurn();
        this.takeMyTurn();
        this.endMyTurn();
    }
}

Notice what this exposes - the monitor isn't "even or odd", it's a entity that is responsible for coordinating the activity of the two threads. You've modeled its internals as true / false, but that's wrong -- it's really two different states that flip back and forth.

public class Monitor {

    private volatile State currentState;
    private final EnumMap<State,State> transitions;

    public boolean isInState(State state) {
       return currentState.equals(state);
    }

    public void changeState() {
        currentState = transitions.get(currentState);
    }

    public enum State {
        Odd, Even
    }    
}

Or better:

public class Monitor<State> {

    private volatile State currentState;
    private final Map<State,State> transitions;

    public boolean isInState(State state) {
       return currentState.equals(state);
    }

    public void changeState() {
        currentState = transitions.get(currentState);
    }   
}

If you imagine something like

interface Task {
    boolean isFinished();
    void doNext();
}

Then it should be clear that your current Even and Odd are both instances of the same class, with a different State and a different Task, which might look like

class CountingTask implements Task {
    private final int [] numbersToCount;
    private int nowAt;

    CountingTask(int [] numbersToCount, int startAt) {
        ...
    }

    boolean isFinished () {
        return nowAt >= numbersToCount.length;
    }

    void doTask () {
        System.out.println(numbersToCount[nowAt]);
        nowAt += 2;
    }
}