Is it OK to have an empty method?

Question

Is it OK to have an empty method and have it overridden in its subclass(es)? This is how it would look like in my code.

public class Rook() {

    public void voidCastleRight() { }

}

public class ShortRook() extends Rook {

    @Override
    public void voidCastleRight() {
        getPlayer().setkSC(false); //void King Side Castling (Short Castle)
    }

}

public class LongRook() extends Rook {

    @Override
    public void voidCastleRight() {
        getPlayer().setqSC(false); //void Queen Side Castling (Long Castle)
    }
}

The context is a chess engine. It is written in Java and it will have to search for the next 'best' move to make given a state of the board. It is therefore important that everything is implemented as efficient as possible because a lot of methods will be called several millions of times. Consequently, I want this hierarchy of Rooks as opposed to one Rook class in which I would have to check on what side the Rook is on and checking if the Rook is on its initial position etc etc..

When a Board is first created, there will be a ShortRook and a LongRook. While the game progresses, it is possible that more Rooks are introduced into the game because of Pawn promotion. These will be instantiations of Rook.

The method voidCastleRight() will be called whenever a Rook is moved. Rooks that exist because of Pawn promotion shouldn't void the castle right when moved (empty method). Rooks that exist since the beginning of the game, should void the castle rights when moved (methods in subclasses).

I have also written a parser which takes FENStrings and converts them to a Board and vice versa. When the Rooks are not on their initial positions, there is no way to tell which are the Short- and LongRook. This isn't a problem because the castle right is voided already anyway and they can be parsed to instantiations of Rook. Therefore would it be OK if I casted a Short- or LongRook object to Rook whenever it has voided the relevant castle right (i.e. it has moved)? This way it won't needlessly void the castle right when it is already voided. I don't care about the complexity of these parser methods as they will not be used in search.

While some might consider these thoughts micro-optimizations "which are the root of all evil", maybe these optimizations will pay off when the method has to be called a few million times. I'm also more concerced about OOP paradigms.

PS: I know Java isn't the best language to use for this application, it is irrelevant. I am aware that object creation (in Java) is expensive. I will make sure no objects are created during the search.

Answer 1

Although it is certainly OK to have an empty method that does nothing, you should evaluate its alternative - an empty method that is missing entirely, i.e. an abstract method.

This may be applicable in your case, because you will be creating a ShortRook, a LongRook, or a promoted Rook:

public abstract class AbstractRook() {
    public abstract void voidCastleRight();
}

public class ShortRook() extends AbstractRook{
    @Override
    public void voidCastleRight() {
        getPlayer().setkSC(false); //void King Side Castling (Short Castle)
    }
}

public class LongRook() extends AbstractRook{
    @Override
    public void voidCastleRight() {
        getPlayer().setqSC(false); //void Queen Side Castling (Long Castle)
    }
}

public class PromotedRook() extends AbstractRook{
    @Override
    public void voidCastleRight() {
        throw new IllegalStateException("Promoted rook cannot castle");
    }
}

Answer 2

The answer to the title of your question:

Is it OK to have an empty method?

Is a resounding yes - this is perfectly acceptable situation.

However - I feel you are using the wrong tool for your requirements. I don't think the functionality of disabling castling should be part of the Rooks function. What you need is a watcher/listener that watches for rook movement and disables castling under the correct situation. You could then use a similar architecture for enabling/disabling taking En-Passant for example.

Also remember Castling that Castling may only be done if the king has never moved, the rook involved has never moved, the squares between the king and the rook involved are unoccupied, the king is not in check, and the king does not cross over or end on a square in which it would be in check. You could implement this whole rule in one place using listeners on the rooks, the kings and the move engine.

Answer 3

I would consider a non-abstract method that does nothing to be a code smell.

The first alternative here would be to create a Rook interface instead of a class.

The second alternative is one you would consider if you wanted to give your base class concrete functionality, perhaps defining the getPlayer() method you reference, then use an abstract class and mark the method as abstract.

Unrelated to your question, but I would think about two other aspects of your code. First is using the name "void" in your method, which is a bit awkward.

Lastly, I usually tell people to reconsider the use of void functions. The reasons is that void functions, by definition, do nothing or perform a side effect. Side effects aren't bad per se but since it involved modifying state, it can make your program harder to reason about.

For example, consider a method validMoves() that returns the list of valid moves that the piece can make. In this case, you could just have a Piece interface and treat every piece the same when evaluating possible moves:

interface Piece {
    List<Move> validMoves();
}

In this case you would have just one instance class of Rook; one that is on either side of the king. You don't have to define special methods for Kings, Rooks and Pawns because they'd each know the moves that they could make.

This would considerable flatten your class hierarchy and make processing and evaluating the board easier to reason about.