Replacing switch statement with HashMap

Question

I have a socket class which receives packages (byte arrays). Every package has an integer in order to identify its type. Every case in the switch statement looks like this:

switch(packet.getHeader().getID()) {
case PingRequest.ID:
    if(data.length != PingRequest.SIZE)
        return null;

    PingRequest pingRequest = new PingRequest(data); /* specific parsing of the byte array */
    pingRequest.setHeader(data); /* method of the abstract class */
    packet = pingRequest;
break;
case Error.ID:
    if(data.length != Error.SIZE)
        return null;

    Error error = new Error(data);
    error.setHeader(data);
    packet = error;

    break;
...

Every packet has different information which is why every packet has a different constructor (creates the packet members from the byte array data)

Since every case looks somewhat similar (and there are a lot of them) I thought I should optimize it by using a HashMap:

public static HashMap<Integer, Class<?>> _packetTypes = new HashMap<Integer, Class<?>>();
_packetTypes.put(PingRequest.ID, PingRequest.class);

What I now wanted to achieve was something like this:

Class<?> myClass = PacketAbstract._packetTypes.get(packet.getHeader().getID());

Class[] cArg = new Class[1];
cArg[0] = Byte.class;
Constructor<?> ct = myClass.getConstructor(cArg);

/* Doesn't make any sense from here on */
myClass specialPacket = ct.newInstance(data); 
specialPacket.setHeader(data);
packet = specialPacket;

So the basic idea was to create a hashmap which contains the packet id and the corresponding packet class which would allow me to create the specialized packet. The last code section was meant to replace my switch statement.

Question:

Is my idea how to solve this correct? If not please enlighten me. How do I implement the last part of my code which doesn't make sense so far (if this is the right way to go)?

EDIT: packet is an object of the abstract class Packet which gets extended by every special packet like PingRequest

Rob I · Answer 1 · 2013-05-01T13:45:14.290

It seems like your idea would work. I think the code would need to be something like this:

Class<? extends Packet> myClass = PacketAbstract._packetTypes.get(packet.getHeader().getID());

Constructor<Packet> ct = myClass.getConstructor(data.getClass());
Packet specialPacket = ct.newInstance(data);
specialPacket.setHeader(data);
packet = specialPacket;

However, using reflection is usually a last resort in my opinion. Consider looking into other patterns such as the Visitor Pattern which may help this type of problem.

Another option would be to embed the correct concrete Class of each identifier within an enum, something like this:

enum PacketID {
    private int id;
    private int size;
    private Class<? extends Packet> subClass;
    private PacketID(int id, int size, Class<? extends Packet> subClass) {
        this.id = id;
        this.size = size;
        this.subClass = subClass;
    }

    ERROR(  1, 100, Error.class),
    PINGREQ(2,  25, PingRequest.class);
}

That would also allow you to abstract away that size check, which I didn't see in your first HashMap solution, as well as potentially skip the HashMap altogether.

EDIT: changed parameter to getConstructor() since it appears you use byte arrays rather than just a Byte. EDIT: added a second alternate solution using enum fields.

johnchen902 · Answer 2 · 2013-05-01T13:41:46.470

0

Class<?> myClass = PacketAbstract._packetTypes.get(packet.getHeader().getID());

Class[] cArg = new Class[1];
cArg[0] = byte[].class; // not Byte.class
Constructor<?> ct = myClass.getConstructor(cArg);

Object specialPacket = ct.newInstance(data);
Method mt = myClass.getMethod("setHeader", byte[].class)
mt.invoke(specialPacket, data);
packet = specialPacket;

edited May 01 '13 at 13:41

answered May 01 '13 at 13:27

johnchen902

9,531
1
27
69

`Class.newInstance()` would work, but the code would have to be changed to use default constructors for these subclasses. – Rob I May 01 '13 at 13:38
@RobI oops... It's a mistake. Fixed. – johnchen902 May 01 '13 at 13:42

GJ13 · Answer 3 · 2013-05-01T16:04:36.183

0

Could you maybe use Factory Pattern(example at http://alvinalexander.com/java/java-factory-pattern-example) ?

So something like:

interface Packet {
     void parse(Datatype data);
     void setHeaders (Datatype data);
}

class PingPacket implements Packet {
    void parse (Datatype data) {
      ....
      ....
    }

    void setHeaders(Datatype data) {
     ...
     ...
    }
}

class ErrorPacket implements Packet {
    void parse (Datatype data) {
     .....
     .....
    }

    void setHeaders(Datatype data) {
     ...
     ...
    }
}

class PacketFactory {
  Packet getInstance(int packetId) {
     if (packetId == Ping_ID) {
         ...
         ...
         return new PingPacket();
     } else if (packetId == ERROR_ID) {
        ...
        ...
        return new ErrorPacket();
       }
  }
}

edited May 01 '13 at 16:04

answered May 01 '13 at 13:49

GJ13

488
3
11

Then I would have to write hundreds of `if (packetId == Ping_ID)` for each id one if statement, which would be the same as using the switch statement. – May 01 '13 at 14:45
yes, you will have to write many if statements but you end up avoiding the repeated blocks of code in each case statement of switch – GJ13 May 01 '13 at 15:49
Or maybe you could modify PacketFactory getInstance(int packetId), so that it finds the subclass name from a HashMap(which has to be constructed with key as packetId and value as classname) and then use the answer specified in stackoverflow question http://stackoverflow.com/a/4767105/2040095 to instantiate class from string – GJ13 May 01 '13 at 15:57

Replacing switch statement with HashMap

3 Answers3