1
\$\begingroup\$

I would like to refactor this code printing odd and even numbers with two threads considering agile practice:

public class EvenOddNumberPrinter {

    private static class NumberPrinter {

        // To check if even number is printed or not.
        private boolean isEvenPrinted = true;

        public void printOdd(int number) throws InterruptedException {
            // Get a lock on NumberPrinter
            synchronized (this) {
                // Wait until even is not printed.
                if (!isEvenPrinted)
                    wait();

                System.out.println(number);
                isEvenPrinted = false;

                // Notify the other waiting thread which is waiting on
                // NumberPrinter
                // Other thread will get out of waiting state
                notify();
            }
        }

        public void printEven(int number) throws InterruptedException {
            synchronized (this) {
                if (isEvenPrinted)
                    wait();

                System.out.println(number);
                isEvenPrinted = true;
                notify();
            }
        }
    }

    private static class OddNumberGenerator implements Runnable {

        private NumberPrinter q;
        private int max;

        public OddNumberGenerator(NumberPrinter q, int max) {
            this.q = q;
            this.max = max;
        }

        @Override
        public void run() {
            for (int i = 1; i < max; i = i + 2) {
                try {
                    q.printOdd(i);
                } catch (InterruptedException ex) {
                    ex.printStackTrace();
                }
            }
        }
    }

    private static class EvenNumberGenerator implements Runnable {

        private NumberPrinter printer;
        private int max;

        public EvenNumberGenerator(NumberPrinter printer, int max) {
            this.printer = printer;
            this.max = max;
        }

        @Override
        public void run() {
            for (int i = 2; i <= max; i = i + 2) {
                try {
                    printer.printEven(i);
                } catch (InterruptedException ex) {
                    ex.printStackTrace();
                }
            }
        }
    }

    public static void main(String[] args) {
        int maxNumber = 10;
        NumberPrinter printer = new NumberPrinter();
        new Thread(new EvenNumberGenerator(printer, maxNumber)).start();
        new Thread(new OddNumberGenerator(printer, maxNumber)).start();
    }

}
\$\endgroup\$
3
\$\begingroup\$

Inconsistency

In OddNumberGenerator your printer is called q. In EvenNumberGenerator it is called printer. Why the difference?

The odd one prints to max-1 but the even one prints to max. They should both print to max, otherwise you are assuming that max must be even.

No check after wait

After your wait() call, you don't check to see if you are in the correct state before printing the next number. If both threads called printOdd(), it would happily print two or more odd numbers in a row. It's not a problem with your program because your two threads are well behaved, but it isn't a very good example of using synchronization.

Simplification

You don't need separate printOdd() and printEven() functions that do almost the same thing. You could combine them into a single printNumber() function.

Similarly, you don't need two number generators. If you just added a start variable to the number generator, you could create an odd generator with a start value of 1 and an even generator with a start value of 2.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your comment. I Will redactor it accordingly. I would like to know whether all oops concept is followed properly. \$\endgroup\$ – Pratik Suman Jan 24 '16 at 12:49

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.