Discussion Gamepackethandler

[Szakalaka]

Hello. Been lookin inside l2j sources for inspiration for own project ( no i, dont write lineage emulator, just and abstract universal base i made my authserver on ) and i stumbled upon the GamePacketHandler.java

 

Thats a decent factory pattern but the switch case CONNECTED AUTHED IN_GAME does not fit here.

So whats my problem? You switch the client state and so the factory goes to right "subfactory under the case". Thats definitely should be an abstarct factory and instead of the switch case state you should instantiate a conrete factory for every state. What do you think about it?

Greetz
 

[Tryskell]

You can make 3 different, overidden methods according cases and call them accordingly, but even as a comprehensive point of view that won't change a lot, ingame packets being 90% of packets.

 

Both on readable and performance views, you got no interests to do that, at least for L2 case. That's a switch drop for 3 classes/methods instead of one. And we speak of 9 packets for IL case.

Edited May 18, 2016 by Tryskell

[Sdw]

Drop that whole class and make a big ass enum :D

[Szakalaka]

Drop that whole class and make a big ass enum :D

Im afraid we dont understand eachother. But it happens sometimes :)

 

And to Tryskell - i would like to completly get rid of the switch between 3 states. You can instantiate concrete factory instead of changing the state enum and then switch casing esecially when 99% packtes are ingame packets. Everytime a game packet comes it checks "hey maybe in im connected on authed state" ? NO.

Edited May 18, 2016 by Szakalaka

[Sdw]

I understand myself that's fine.

[Tryskell]

Drop that whole class and make a big ass enum :D

 

Similar to that ? (Simple copy paste from stackoverflow)

enum Type {
    Y_TYPE(1) {
        @Override
        public X createInstance() {
            return new Y();
        }
    }, Z_TYPE(2) {
        @Override
        public X createInstance() {
            return new Z();
        }
    };

    private int id;

    private Type(int id) { 
        this.id = id; 
    }

    public abstract X createInstance();
}

[Szakalaka]

They say noone understands good programmers. And noone seems to understand me :)

 

GamePacketHandler implements some abstract factory. And inside it has a switch based on cases lol. Every case should be another factory and the main GamePacketHandler should be set to factory of current state. Thats my point of view.

[Sdw]

Damn I don't have the class anymore here, mind gisting/pastin whatever it ?

 

And no Tryskell we took a different approach, which I guess won't work with regular mmocore since it reuse some netty capability.

[Tessa]

It will still need to check the state in the abstract factory. The only difference is that, you now have to instantiate one more object (e.g. the appropriate factory). :lol:

Sure, it's more readable that way.

[Szakalaka]

No!! You instantiate the factory and the "guy who gets packet to pass to factory" passes it to ABSTARCT factory which is instantiated to CONCRETE instance of current state! Oh damn.  Well it works on my server. I dont want you to redesign anything - jsut asked for opinion

[Nik]

public enum IncomingPackets implements IIncomingPackets<L2GameClient>
{
	LOGOUT(0x00, Logout::new, ConnectionState.AUTHENTICATED, ConnectionState.IN_GAME),
	ATTACK(0x01, Attack::new, ConnectionState.IN_GAME),
	REQUEST_START_PLEDGE_WAR(0x03, RequestStartPledgeWar::new, ConnectionState.IN_GAME),
	REQUEST_REPLY_START_PLEDGE(0x04, RequestReplyStartPledgeWar::new, ConnectionState.IN_GAME),
	REQUEST_STOP_PLEDGE_WAR(0x05, RequestStopPledgeWar::new, ConnectionState.IN_GAME),
	REQUEST_REPLY_STOP_PLEDGE_WAR(0x06, RequestReplyStopPledgeWar::new, ConnectionState.IN_GAME),
	REQUEST_SURRENDER_PLEDGE_WAR(0x07, RequestSurrenderPledgeWar::new, ConnectionState.IN_GAME),
	REQUEST_REPLY_SURRENDER_PLEDGE_WAR(0x08, RequestReplySurrenderPledgeWar::new, ConnectionState.IN_GAME),
	REQUEST_SET_PLEDGE_CREST(0x09, RequestSetPledgeCrest::new, ConnectionState.IN_GAME),
	REQUEST_GIVE_NICK_NAME(0x0B, RequestGiveNickName::new, ConnectionState.IN_GAME),
	CHARACTER_CREATE(0x0C, CharacterCreate::new, ConnectionState.AUTHENTICATED),
	CHARACTER_DELETE(0x0D, CharacterDelete::new, ConnectionState.AUTHENTICATED),
	PROTOCOL_VERSION(0x0E, ProtocolVersion::new, ConnectionState.CONNECTED),
......

such kind of enum...

1st value is the packet optype, 2nd value is the class file factory, the next ones are just in which states the packet should be active.

Edited May 18, 2016 by Nik

[Sdw]

Cmon !

[Szakalaka]

Are you serious with this enum... You are thinkin completly oposite way than me xD Nevermind guys.

[Sdw]

Yes we are, you don't even know what we do with it lol

 

Anyway, I checked and it's netty specific, nvm as you said :D

[Szakalaka]

The idea of abstarct factory is to skip IF or SWITCH CASE checks for current state. The switch case by opcodes for concrete factory is fine and the way to create concrete packet.

Instead of having the enum with state abstract factory would be instantiated to concrete one with current state. And voila, if u receive a packet from game it will NOT get checked "am i in auth? am i in connected? " NO because the factory would be set.

 

Thats enough thats for feedback :D