Java - Instantiation

Question

I'm trying to create a new class of something I'm not entirely sure of the class type. This could be better explained with my code:

private static Class[] Packets = new Class[]
            {
                KeepAlivePacket.class, // 0x00
                LoginRequestPacket.class, // 0x01
                HandshakePacket.class, // 0x02
                    }
.......

class HandshakePacket extends TCPPacket
{
    public HandshakePacket()
    {

    }
    byte protocolVersion;
    String username;
    String host;
    int port;
    @Override
    public void writePacketData(DataOutputStream os) throws IOException {
        os.write(id);
        os.writeByte(protocolVersion);
        writeString(os, username);
        writeString(os, host);
        os.writeInt(port);
    }
    @Override
    public void readPacketData(DataInputStream is) throws IOException {
        protocolVersion = is.readByte();
        username = readString(is,16);
        host = readString(is,16);
        port = is.readInt();
    }
    @Override
    public void setId(byte id)
    {
        this.id = id;
    }
}

.......
    public static TCPPacket getNewPacket(int i)
    {
    try
    {
        Class var1 = (Class)Packets[i];
        return var1 == null ? null : (TCPPacket)var1.newInstance(); <-- error on this line
    }
    catch (Exception var2)
    {
        var2.printStackTrace();
        System.out.println("Skipping packet with id " + i);
        return null;
    }
}

and for anyone wondering what TCPPacket is:

package vc.voidwhisperer.proxy.packet;

import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.IOException;

public class TCPPacket {
    public TCPPacket()
    {

    }
    public byte id = 0;
    public void writePacketData(DataOutputStream os) throws IOException
    {

    }
    public void readPacketData(DataInputStream is) throws IOException
    {

    }
    public void setId(byte id)
    {

    }
}

As you can see I'm trying to instaniate a new object that I can't be entirely sure of what the class type is. However, it's spitting out this exception:

java.lang.InstantiationException: vc.voidwhisperer.proxy.packet.Packet$HandshakePacket
at java.lang.Class.newInstance0(Unknown Source)
at java.lang.Class.newInstance(Unknown Source)
at vc.voidwhisperer.proxy.packet.Packet.getNewPacket(Packet.java:2509)
at vc.voidwhisperer.proxy.UserConnection.run(UserConnection.java:52)

The InstantiationException wraps the true exception which is usually printed after it. Can you provide the actual exception? It also says the exception occurs calling the constructor of Packet.HandshakePacket. Can you provide the source code for the class which causes the exception and tell use exactly which line it occurred on? — Peter Lawrey, Dec 28 '12 at 17:08
BTW Is there any reason that `setId` doesn't set the `id` field? — Peter Lawrey, Dec 28 '12 at 17:09
Include relevant code from `HandshakePacket` to your question, including its constructors. I would recommend copy/pasting the code (and not typing it manually into the question). — Perception, Dec 28 '12 at 17:12
@PeterLawrey I think the contents of TCPPacket have been hidden for sake of space... — Shotgun Ninja, Dec 28 '12 at 17:12
@ShotgunNinja I thought of that but his subclasses override `setId` so it can be set (in any case it doesn't save any lines ;) — Peter Lawrey, Dec 28 '12 at 17:13
Also while `id` defined in TCPPacket it is not marshalled by this class but by subclasses and it is written but not read. .. — Peter Lawrey, Dec 28 '12 at 17:15
I would also use DataOutputStream.writeUTF and DataInputStream.readUTF instead of creating a new way to write Strings which I suspect is fixed width. — Peter Lawrey, Dec 28 '12 at 17:16
it may help to find the root cause if you use generics, i.e. 'Class[] Packets' or 'Class extends Packet>[] Packets' ... how are you filling that array ? — jambriz, Dec 28 '12 at 17:18
@PeterLawrey i don't have a choice, this is taking packets from someone else's protocol :/ — VoidWhisperer, Dec 28 '12 at 17:19
I notice that `HandshakePacket` is a non-public class (it has default visibility). Is it possible that you have a visibility problem? — Christopher Schultz, Dec 28 '12 at 17:43

score 2 · Answer 1 · answered Dec 28 '12 at 17:14

2

Reflection is overkill for this.

Just do

switch (i) {
  case 0: return new KeepAlivePacket();
  case 1: return new LoginRequestPacket();
  case 2: return new HandshakePacket();
  default: throw new IllegalArgumentException();
}

and ideally replace i with an enum.

That will get you the advantages of static type and signature checks making your code more maintainable, and avoids all the reflective guff that masks exceptions.

answered Dec 28 '12 at 17:14

Mike Samuel

118,113
30
216
245

There's roughly 200 packets. It's not overkill :/ – VoidWhisperer Dec 28 '12 at 17:16
1

Meh, trading code for memory is a long-debated issue. If you were to read-in configuration from a file, then you can make a better argument for using data-based dispatching instead of code-based dispatching. – Christopher Schultz Dec 28 '12 at 17:41
@VoidWhisperer, The switch approach wins for large hand-coded arrays. A dense int switch has zero per-case overhead since it just compiles to a single bounds check and `jsr` instruction. Each case statements that returns the result of a void constructor invocations take 3 bytes each for the `new` instruction and 1 for the return. The size of the static initializer that sets up a 200 element array is significantly larger since each aastore requires 3 other instructions to set up the stack : `a = new Class[] { x, y, z }` actually desugars to `a = new Class[3]; a[0] = x; a[1] = y; a[2] = z;`. – Mike Samuel Dec 28 '12 at 18:24
@VoidWhisperer, runtime-wise, the switch statement and ctors can be inlined by the JIT but reflective uses cannot so incur runtime overhead and unpredictable branches every time. – Mike Samuel Dec 28 '12 at 18:24

score 2 · Answer 2 · edited Dec 28 '12 at 17:59

2

You are trying to instantiate a non-static inner Class from within a static method. Creating a new HandshakePacket object requires a surrounding Packet object as it's parent, and you aren't providing one.

So, either make HandshakePacket a static inner class, make getNewPacket a non-static method, or create a new Packet object to use as a parent for your new HandshakePacket object.

edited Dec 28 '12 at 17:59

Mike Samuel

118,113
30
216
245

answered Dec 28 '12 at 17:46

Christopher Schultz

20,221
9
60
77

@VoidWhisperer, if you're going to reflectively invoke an inner-class constructor, you need to pass the outer instance as the first argument since `outerInstance.new NonStaticInnerClass()` desugars to `new OuterClass$NonStaticInnerClass(outerInstance)` and `new NonStaticInnerClass()` is really `this.new NonStaticInnerClass()` so desugars to `new OuterClass$NonStaticInnerClass(this)`. `Class.newInstance()` won't work. You need to use [`java.lang.reflect.Constructor.newInstance`](http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/Constructor.html#newInstance%28java.lang.Object...%29). – Mike Samuel Dec 28 '12 at 18:41
Your only change was to make `getNewPacket` non-static? Is `Packet` the containing class for `HandshakePacket`? It's not clear from your code that it is -- I just assumed it was the case. – Christopher Schultz Dec 28 '12 at 19:28

Java - Instantiation

2 Answers2