3
\$\begingroup\$

I've just implemented a simulation of tower of hanoi in Java. Any suggestions are welcomed. Here's my code:

static class Node{
    int data;
    Node next;

    public Node(int data){
    this.data = data;
    }
}


static class Stack{
    Node top;
    String stackName;

    public Stack(String name){
        stackName = name;
    }

    public String getStackName(){
        return stackName;
    }

    public void push(int data){
        if(top==null){
            top = new Node(data);
            System.out.println("Created Stack, pushing : " + data);
        }else{
            System.out.println("Pushed : " + data);
            Node someNode = new Node(data);
            someNode.next = top;
            top = someNode;
        }
    }

    public Node pop(){
        if(top==null){
            System.out.println("Error : Stack is empty!");
            return new Node(-1);
        }
        if(top.next==null){
            Node toRet = new Node(top.data);
            System.out.println("Stack " + this.stackName + " will be emptied now");
            top = null;
            return toRet;
        }
        Node temp = top;
        top = top.next;
        System.out.println("Popped : " + temp.data);
        return temp;
    }

    public String getTop(){
        if(top==null){
            return "Stack is empty";
        }
        return String.valueOf(top.data);
    }

    public void pushTo(Stack anotherStack){
        System.out.println("Pushed " + this.top.data + " from " + this.stackName + " to " + anotherStack.stackName);
        Node toPush = this.pop();
        anotherStack.push(toPush.data);
    }

}


public static void tower(int number, Stack stackFrom, Stack stackInter, Stack stackTo){
    if(number == 1){
        stackFrom.pushTo(stackTo);
    }else{
        tower(number-1, stackFrom, stackTo, stackInter);
        stackFrom.pushTo(stackTo);
        tower(number-1, stackInter, stackFrom, stackTo);
    }
}

public static void main(String[] args) {
    Stack myStack = new Stack("A");
    myStack.push(10);
    myStack.push(15);
    myStack.push(20);
    myStack.push(25);
    Stack anotherStack = new Stack("B");
    Stack yetAnotherStack = new Stack("C");
    tower(4, myStack, anotherStack, yetAnotherStack);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Suggestion: explicitly state your goals - in particular comment your code. \$\endgroup\$ – greybeard Jan 9 '16 at 7:58
7
\$\begingroup\$

Use java.util.Deque

Instead of implementing your own stack, it's better and easier to use the Deque in the JDK. (Not Stack, as the JavaDoc explains, Deque is recommended. Thanks @greybeard for the tip!)

Naming

In this game there are discs and towers (or sticks, rods, pegs). It would be better to call them that way instead of Node and Stack.

tower is a poor name for moving discs. In general, verbs are best for method names. In this example move would be natural.

Alternative implementation

With the above suggestions applied, the implementation becomes:

import java.util.ArrayDeque;
import java.util.Deque;

public class Hanoi {

    private static class Tower {

        private final String name;
        private final Deque<Integer> stack;

        public Tower(String name) {
            this.name = name;
            this.stack = new ArrayDeque<>();
        }

        public void add(int... discs) {
            for (int disc : discs) {
                stack.push(disc);
            }
        }

        public int size() {
            return stack.size();
        }

        public void moveOneDisc(Tower to) {
            System.out.println("Pushed " + this.stack.peek() + " from " + this.name + " to " + to.name);
            to.stack.push(this.stack.pop());
        }

        private void moveDiscs(int count, Tower mid, Tower to) {
            if (count == 1) {
                moveOneDisc(to);
            } else {
                moveDiscs(count - 1, to, mid);
                moveOneDisc(to);
                mid.moveDiscs(count - 1, this, to);
            }
        }
    }

    public static void main(String[] args) {
        Tower from = new Tower("A");
        from.add(10, 15, 20, 25);
        Tower mid = new Tower("B");
        Tower to = new Tower("C");
        from.moveDiscs(from.size(), mid, to);
    }
}

Some other improvements:

  • Moved the tower method inside the Tower class and renamed to move
  • Tower.add accepts varargs, for conveniently adding multiple discs
  • Tower.size to know the number of discs on a tower
  • The method names of Tower are all in the language of the problem domain, not the implementation (implementation details are well hidden)
\$\endgroup\$
  • \$\begingroup\$ From the Java SE docs: prefer Deque over Stack, the crux being legacy classes won't go away, hogging names that would make perfect interface names. Toyed with the code presented: Lucas on Ideone. Not convinced moveTo() should be a member of Tower, let alone move(). Like if (0 < count) { move(count-1, …); trace(); to.push(from.pop()); move(count-1, …); } better. \$\endgroup\$ – greybeard Jan 9 '16 at 12:29
  • \$\begingroup\$ @greybeard Aw, I always forget that about Stack, thanks for the reminder. The example implementation is not really meant to be perfect. But inspired by your work on Ideone I added some improvements, which you were right to point out. Btw I disagree with Tower extending ArrayDeque, because a Tower is not meant as an ArrayDeque. So I think composition is better here than inheritance. \$\endgroup\$ – janos Jan 9 '16 at 12:55

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.