Java Clean Code, use of enums in data structure

Question

I have defined a data structure in Java that allows to manage certain type of elements like a Queue, with the special feature of using 2 inner rows (windows) to attend elements:

import java.util.ArrayList;

public class TwoWindowsQueue<E> {
    enum WindowState {CLOSED, OPEN}
    
    enum WindowTurn {WINDOW1, WINDOW2}

    private ArrayList<E> window1, window2;
    private WindowState windowState1, windowState2;
    private WindowTurn turn;

    public TwoWindowsQueue(E[] elements) {
        setWindowState1(WindowState.OPEN);
        setWindowState2(WindowState.OPEN);
        setTurn(WindowTurn.WINDOW1);

        for (E elem : elements) {
            insert(elem);
        }
    }

    public WindowTurn getTurn() {
        return turn;
    }

    private void setTurn(WindowTurn turn) {
        this.turn = turn;
    }

    public void closeQueue() {
        if (pendingElementsInQueue() > 0)
            throw new IllegalStateException("Elements are pending");
        setWindowState1(WindowState.CLOSED);
        setWindowState2(WindowState.CLOSED);
    }

    private void setWindowState1(WindowState state) {
        windowState1 = state;
    }

    private void setWindowState2(WindowState state) {
        windowState2 = state;
    }

    public boolean isWindow1Open() {
        return windowState1 == WindowState.OPEN;
    }

    public boolean isWindow2Open() {
        return windowState2 == WindowState.OPEN;
    }

    public void closeWindow() {
        if (pendingElementsInWindow1() <= pendingElementsInWindow2()) {
            transfer(window1, window2, pendingElementsInWindow1());
            setWindowState1(WindowState.CLOSED);
            setTurn(WindowTurn.WINDOW2);
        } else {
            transfer(window2, window1, pendingElementsInWindow2());
            setWindowState2(WindowState.CLOSED);
            setTurn(WindowTurn.WINDOW1);
        }
    }

    public void openWindow() {
        if (windowState1 == WindowState.CLOSED) {
            setWindowState1(WindowState.OPEN);
            return;
        }
        if (windowState2 == WindowState.CLOSED)
            setWindowState2(WindowState.OPEN);
    }

    public boolean isInQueue(E elem) {
        return window1.contains(elem) || window2.contains(elem);
    }

    public int pendingElementsInQueue() {
        return pendingElementsInWindow1() + pendingElementsInWindow2();
    }

    public int pendingElementsInWindow1() {
        return window1.size();
    }

    public int pendingElementsInWindow2() {
        return window2.size();
    }

    public void insert(E elem) {
        if (elem == null) throw new IllegalArgumentException("null");
        validateWindowsClosed();
        if (isInQueue(elem))
            throw new IllegalStateException("Duplicate element");

        if (isWindow1Open() && isWindow2Open()) {
            if (pendingElementsInWindow1() <= pendingElementsInWindow2()) {
                window1.add(elem);
            } else {
                window2.add(elem);
            }
            return;
        }
        if (isWindow1Open()) window1.add(elem);
        if (isWindow2Open()) window2.add(elem);
    }

    public boolean isQueueClosed() {
        return !isWindow1Open() && !isWindow2Open();
    }

    private void validateQueueEmpty() {
        if (pendingElementsInQueue() == 0)
            throw new IllegalStateException("Queue is empty");
    }

    private void validateWindowsClosed() {
        if (isQueueClosed())
            throw new IllegalStateException("Both windows are closed");
    }

    public E elementToServe() {
        validateQueueEmpty();
        validateWindowsClosed();
        if (getTurn() == WindowTurn.WINDOW1) {
            return window1.get(0);
        } else {
            return window2.get(0);
        }
    }

    public void serve() {
        validateQueueEmpty();
        validateWindowsClosed();
        if (getTurn() == WindowTurn.WINDOW1) {
            window1.remove(0);
            if (isWindow2Open()) setTurn(WindowTurn.WINDOW2);
            return;
        } else {
            window2.remove(0);
            if (isWindow1Open()) setTurn(WindowTurn.WINDOW1);
        }
    }

    public void balance() {
        if (!isWindow1Open() || !isWindow2Open())
            throw new IllegalStateException("Cannot balance");
        int diff = pendingElementsInWindow1() - pendingElementsInWindow2();
        if (diff > 1) {
            transfer(window1, window2, diff / 2);
        } else if (diff < -1) {
            transfer(window2, window1, diff / 2);
        }
    }

    private void transfer(ArrayList<E> source, ArrayList<E> destination, int amount) {
        while (amount > 0) {
            destination.add(source.get(source.size() - amount));
            source.remove(source.size() - amount);
            amount--;
        }
    }
}

As you can see, I have used enums in an attempt to make the code easier to understand and maintain. But my question is, for the windows state and turn, is there any other alternative to enums that is Clean Code friendly?

Also, if enums are the only option, is there any way to simplify its usage?

Answer 1

Let me start off by addressing this point.

As you can see, I have used enums in an attempt to make the code easier to understand and maintain. But my question is, for the windows state and turn, is there any other alternative to enums that is Clean Code friendly?

Enums are about as "Clean Code friendly" as you can get. They are explicit and clear, easy to understand, and they give you both Value Safety and Exhaustiveness Checking. They are powerful, and you should use them any chance that you get. Especially if you are coding Java, because Java enums are more powerful than any other (non-JVM) languages enumerated values.

Now, there is still some room for improvement.

For example, you have enum WindowTurn, but yet, you write private WindowState windowState1, windowState2;? This is unwise because, what happens if you add a new value to WindowTurn? There are all these places that need to be updated. So no, I think your choice to use an enum was very wise, but you are not utilizing it to its full extent.

The proper solution here would be to use a java.util.Map, where the key is WindowTurn, and the value would be some object that handles both WindowState as well as the ArrayList for each Window.

Really, I think that is the biggest flaw in your code -- all of the class will completely fall apart if you decide to add a new WindowTurn. If you were to refactor your solution to do things like I described, you could add a new WindowTurn value, and everything would work with no changes required. That is clean code.

That said, making the change would require you to refactor your API. As is now, this class has methods for 1 and 2. So, if you were to add a 3rd, you would now need a 3 variant for all of the methods. That is not clean code at all. So, instead, all the methods that have a 1/2 variant, change them to take in WindowTurn, and then you can make the logic general enough to apply to the relevant WindowTurn.

Again, the key concept here is that you should be able to take WindowTurn and either add or remove values from it, and the class should just work. Not only is that clean, it's maintainable.

---

Let me finish by addressing your points directly now.

We needed to understand the above concepts, but now that we do, we can answer your questions simply.

As you can see, I have used enums in an attempt to make the code easier to understand and maintain.

Indeed you have, but you are only partway there, for reasons mentioned above.

But my question is, for the windows state and turn, is there any other alternative to enums that is Clean Code friendly?

There certainly are, but I see nothing else that would be as "Clean Code friendly" as using an enum. Like I said, you are not using enums to their full capability, and for that reason, you are losing out on "cleanliness". But an enum used correctly? In my opinion, using an enum correctly is cleanliness at its MAXIMUM. At least for this code example.

Also, if enums are the only option, is there any way to simplify its usage?

Like I said, using them correctly. All mentioned above.

Answer 2

It is important to understand one thing about Clean Code. Clean Code gives people a convenient and easy to remember abstraction when they want to talk about good programming practises. But when you ask them for specifics about what principles they actually follow, you get a lot of muttering, hand wringling and looking at the ceiling and suddenly people remember that they were supposed to have a cigarette break outside or that they left the kettle on. In my experience 50% of the people who talk about Clean Code do not follow half of the guidelines listed in the book. And the other 50% performs even worse. (Also, 73.6% of all statistics are made up, but that's another discussion completely.) Unfortunately the example code you provided falls in the latter category. So, forget the name Clean Code completely, never use it again, burn the book and concentrate on actual concrete principles that produce good code.

Ok, now that we have my opinions about [redacted] out of the way, let's look at your code. This review will be a bit rambling because, and I am sorry for being very blunt here, your code is not salvageable with minor changes, it requires a complete rewrite. But that happens sometimes, it's part of the learning process.

Follow standard conventions.

There are known conventions when it comes to queues. The Java standard library provides a Queue class with method names that are known to most Java programmers. When you implement a queue tyou should follow the same naming conventions and behaviours. If possible, implement standard interfaces like Collection or Iterable.

Choose descriptive and unambiguous names.

This is related to the previous one. "Window" is not a known concept within queues. Windows are portals through which light travels or a view to something that is on the other side of the glass. Windows are not something you store things in. Maybe you should have used "sub-queue" instead of "window"? And once you have done that, document the reason why this bizarre data structure needs to exist. The code explains nothing and I have no idea why you wrote it.

The windowState1 is an ambiguous name. The sub-queue does not have states other than open/closed. Therefore the name should have been isSubQueueOpen and a simple primitive boolean would have been the suitable type.

Single responsibility principle

Your queue class is responsible for maintaining the queue and juggling the sub-queues. That's two things and two is more than one. Split the sub-queues into separate classes that know whether they are open or closed. Implement the open and close methods to the SubQueue class and in the close method, provide the reserve queue to which the elements are moved:

/**
 * Move all pending elements to the reserveQueue and close this sub-queue.
 */
void close(SubQueue reserveQueue)

Don't be redundant. Don't add obvious noise. Reduce complexity as much as possible.

Listing the sub-queues as enum values is redundant and makes the code complicated. You have to make unnecessary if-checks based on the enum value that you could have avoided by replacing the second enum with a variable that points to the sub-queue that is next in turn. Instead of listing the sub-queues as separate variables, you could store them in a list and make the next-in-turn an index pointer to that list. Should you add more sub-queues later, just increase the list size by one with no changes to any other code needed.

Use correct data types

A Java ArrayList is just about the worst data type for a queue. Adding elements to the queue is O(1) but removing an element is O(N). Since you never need to quickly access an individual value from the middle of the queue, a Queue or a LinkedList would have been correct choices.

Use correct exception types

Throwing an IllegalArgumentException("null") is quite an obvious sign of wrong exception. Use NullPointerException. It exists for this exact purpose.

Throwing an IllegalStateException when trying to insert a duplicate value is a surprise. The standard Java data structures return a false boolean value when a modification operation does not alter the target structure. You should follow the same principle.

Avoid negative conditionals.

The isQueueClosed() method checks for negative condition "closed". This will lead to statements like if (!isQueueClosed()) which are quite hard for humans to follow. It is better to use positive conditions such as public boolean isOpen() { ....

You are calling validateQueueEmpty and validateWindowsClosed correctly as fail-fast checks in the beginning of other methods but their names are cpompletely cbackwards. For example ensureQueueHasData and ensureQueueIsOpen are better choices.

And to finally answer your original question: unfortunately no, the way you used enums did not follow the programming guidelines you were interested in.

Answer 3

Have you concidered to use boolean (not Boolean but boolean)?

Example:

private boolean stateClosed; // or window1stateClosed if you like

A few advantages:

1. You have no native support for the undefined value null (principle of least surprise).
2. You have smaller payload because it is an atomic value.
3. You have a value as default: false. (means open by default).
4. It is simple (KISS)
5. You do not need a custom deserializer via OIS (ObjectInputStream) making sure the state is not null.
6. Using an enum the wrapper can be: right, wrong, broken (because null-value). Using a boolean the wrapper can be: right, wrong. But not broken. So it is more not-contradictive (some aspects of acid principle).