Is there anything wrong with my Factory class?

Question

class PieceFactory {     
     @SuppressWarnings("rawtypes")
     public Piece createPiece(String pieceType) throws Throwable{
        Class pieceClass = Class.forName(pieceType);
        Piece piece = (Piece) pieceClass.newInstance();

         return piece;       
     }
}

I'm not all used to handling exceptions yet therefore I'm just throwing them, but everywhere I use a method that uses this factory it tells me I have to throw exceptions like throwable.

For example, in one of my classes I have a method that instantiates a lot of objects using the method that uses the factory. I can use the method in that class by just throwing the exception, however it won't work if I try to pass a reference to that class to another class and then use the method from there. Then it forces me to try catch the exception.

I probably don't need a factory but it seemed interesting and I'd like to try to use patterns. The reason I created the factory was that I have 6 subclasses of Piece and I wan't to use a method to instantiate them by passing the type of subclass I want as an argument to the method.

Answer 1

You are trying to reflectively create a Piece object.

Class.forName() throws ClassNotFoundException, while Class.newInstance() throws InstantiationException, IllegalAccessException (hence why you need to throw Throwable.

A better way to create an object through class types is probably by doing the following:

class PieceFactory {     

    public Piece createPiece(String pieceType) throws Throwable{
        Piece piece = null;

        if ("SubPiece1".equals(pieceType)) {
            piece = new SubPiece1();
        } else if ("SubPiece2".equals(pieceType)) {
            piece = new SubPiece2();
        }

        return piece;       
    }
}

PS, it's untested, just showing a better way to do it.

Hope this helps! :)

Answer 2

Both @SuppressWarnings and throws Throwable ring alarm bells. See Effective Java by Bloch.

Answer 3

If you dont want to declare throws statements or try/catch block everywhere.

Create a class and extend RuntimeException class or use RuntimeException itself or related RuntimeException extending classes.

Then place try-catch block to the root (your factory class methods) and wrap the exceptions thrown into a Runtime exception type (one of above, whichever you choose).

However, it really depends on what you need. At some point you will still need to handle exceptions or your application will throw exception at runtime and app wont do what was expected if you dont handle them.

try{ 
    "your code which throws exception" 
} catch(ThrownExceptionType e){ 
   throw new RuntimrException(e);
}

Remember you still need to handle the exception. It does not mean this is gonna work all well.

Answer 4

Exceptions need to be caught somewhere. If I understood correctly you have one class that is wrapped around this factory, say a FactoryWrapper. Somewhere in your code you use that wrapper class and you are able to either throw you exception or catch it because the method where you are using it is probably surrounded at some point (in a base class probably) by a try/catch block while the other place (to which you are passing your FactoryWrapper reference) is probably a last resort (it is not a good practice, but it could be a method that is never called) where the exception need to be caught.

This is just a possible explanation to why you can not throw the exception in your other class. As other have already mentioned, try not using reflection because that is much slower that the other alternatives.

You could use one abstract factory and then a factory for each type. See details here and here . Hope this helps.

EDIT :

Here is an interesting article about reflection. It will give you an idea when to use it.

Answer 5

I recommend that you work on getting comfortable with writing exception handlers. Otherwise your only option is to riddle your code with throws declarations which becomes annoying and causes difficulties, as you've seen already.

In this case, the body of your method can throw two checked exceptions: ClassNotFoundException from the forname() call, and InstantiationException from the newInstance() call.

How to best handle these depends on your application and your preference. As suggested by another answer, one option is to simply catch them and throw unchecked exceptions. This is sensible if you expect that these exceptions should never occur once the application is complete. This is probably the approach I would take here, since if either of these exceptions occurred it would likely indicate a configuration error.

But if there is reason to think these exceptions could reasonable occur in normal usage, you should think about how you want to handle them. For instance, if the Piece subclass name were being entered into a GUI by a user (not likely, I know, but just to make the point) then a ClassNotFoundException becomes much more likely since the name could easily be misspelled. In that situation, it might make sense to allow this method to throw that exception, and require the caller to catch and handle it (e.g. by providing a message back to the user that the requested class does not exist).

Answer 6

Using reflection like you do is not ideal. As soon as you rename a Piece class and clients pass hardcoded fully-qualified-classnames, your app will break. The Elite Gent's suggestion avoids that problem, but still requires clients to know the concrete class, which is exactly what the factory pattern tries to hide. A better approach in my opinion would be to either use an enum to represent Piece types and let the client pass that as the argument to your factory method, or create separate factory methods for each type (with 6 piece types that is feasible).

Since I am procrastinating anyway, here an example of the enum-approach:

import java.util.ArrayList;
import java.util.List;

class PieceFactoryExample {

    static enum PieceFactory {
        ROOK {
            Piece create() {
                return new Rook();
            }
        };
        abstract Piece create();
    }

    static class Field {
        char line;
        int row;

        public Field(char line, int row) {
            this.line = line;
            this.row = row;
        }

        public String toString() {
            return "" + line + row;
        }
    }

    static interface Piece {

        void moveTo(Field field);

        List<Field> possibleMoves();
    }

    static abstract class AbstractPiece implements Piece {

        Field field;

        public void moveTo(Field field) {
            this.field = field;
        }
    }

    static class Rook extends AbstractPiece {

        public List<Field> possibleMoves() {

            List<Field> moves = new ArrayList<Field>();
            for (char line = 'a'; line <= 'h'; line++) {
                if (line != field.line) {
                    moves.add(new Field(line, field.row));
                }
            }
            for (char row = 1; row <= 8; row++) {
                if (row != field.row) {
                    moves.add(new Field(field.line, row));
                }
            }
            return moves;
        }
    }

    public static void main(String[] args) {

        Piece pawn = PieceFactory.ROOK.create();
        Field field = new Field('c', 3);
        pawn.moveTo(field);
        List<Field> moves = pawn.possibleMoves();
        System.out.println("Possible moves from " + field);
        for (Field move : moves) {
            System.out.println(move);
        }
    }
}

I made everything static here to keep the example self-contained. Normally Field, Piece, AbstractPiece and Rook would be top-level model classes and PieceFactory would be top-level too.

This is a better way to go for your example I think, and avoids the exception handling issue.

Returning to that, there are a couple of approaches you can consider (based on your reflection approach):

Throwing Throwable like you did is bad practice since it lumps together all errors and makes error handling very cumbersome and untransparent for clients. Do not do that unless you have no other options.

Declare 'throws' on your method for all checked exceptions. To decide if this is valid, consider if the client should know and understand the exception type you are throwing. In your example, should the client that wants to create a Rook know and understand the InstantiationException, IllegalAccessException and ClassNotFoundException thrown by your reflection code? Probably not in this case.

Wrap them in a runtime exception which needs not be caught by clients. This is not always a good idea. The fact that code you are calling is throwing checked exceptions usually has a reason. In your example you were doing reflection, and this can go wrong in many ways (the API declares LinkageError, ExceptionInInitializerError, ClassNotFoundException, IllegalAccessException, InstantiationException and SecurityException). Wrapping the checked exceptions in a runtime exception does not make that problem go away. I consider doing this a 'code smell'. If the error means an unrecoverable system failure, then it might be a valid choice, but in most cases you would want to handle such failures more gracefully.

Throw a custom checked exception for a complete subsystem. See for example Spring's org.springframework.dao.DataAccessException which is used to wrap all implementation specific data access exceptions. This means clients will have to catch just one exception type and can figure out the details if they need to. In your case you could create a checked PieceCreationException and use that to wrap all the reflection errors. This is a valid exception handling pattern, but I think it might be a little to heavy-handed for your PieceFactory.

Return null. You could catch all the exceptions within your code and simply return null if they occur. Just make sure your JavaDoc clearly indicates this. A drawback of this approach is that clients might have to check for nulls everywhere.

Return a specific error type. This is a geeky (very object-oriented) approach I saw in the Java core API somewhere a while back (darn, can't remember where). For this you would create an extra Piece type:

class NullPiece implements Piece {
    public void moveTo(Field field) {
        throw new UnsupportedOperationException("The Piece could not be created");
    }
    public List<Field> possibleMoves() {
        throw new UnsupportedOperationException("The Piece could not be created");
    }
}

And return an instance of this type when an error occurs during creation. The advantage is that it is more self-documenting than returning a simple null-value but clients would of course have to check for this using something like instanceof. If they don't, then they will encounter the UnsupportedOperationException thrown when the Piece methods are called. A bit heavy, but very explicit. I'm not sure I would go this far, but it's still an interesting idea.