Using a factory pattern many times [closed]

Question

This is my first question on this website and I hope it goes well.

I have created a Kotlin factory pattern and I am using it very frequently. the code I use it consists of functions inside a class. This is not a static class however, it does not mutate any field inside this class.

When I look at this code, I can tell there is a pattern, so I thought I could improve it. That's why I am asking this question, for ideas & thoughts on how to improve the structure. The code is the following

class Client() {

    fun dispatchFollowEvent(followerID: Int, destination: Server, callback: (success: Any?) -> Unit) {
        val packet: FollowUserPacket = this.factory.makePacket(PacketType.FOLLOW_USER) as FollowUserPacket
        packet.payload = FollowUserPacket.FollowUserPayload(this, followerID)
        sendToServer(packet, this, destination, callback)
    }

    fun dispatchRegisterEvent(destination: Server) {
        val packet: RegistrationPacket = this.factory.makePacket(PacketType.REGISTRATION) as RegistrationPacket
        packet.payload = RegistrationPacket.RegistrationPayload(this)
        sendToServer(packet, this, destination)
    }

    fun dispatchUploadEvent(image: String, destination: Server) {
        val packet: UploadImagePacket = this.factory.makePacket(PacketType.UPLOAD_IMAGE) as UploadImagePacket
        packet.payload = UploadImagePacket.UploadImagePayload(this, image)
        sendToServer(packet, this, destination)
    }

    fun dispatchListUsersEvent(destination: Server, callback: (usersIDs: Any?) -> Unit) {
        val packet: ListUsersPacket = this.factory.makePacket(PacketType.LIST_USER_IDS) as ListUsersPacket
        packet.payload = ListUsersPacket.ListUsersPayload(this)
        sendToServer(packet, this, destination, callback)
    }

    fun dispatchGetFollowRequestsEvent(destination: Server, callback: (requests: Any?) -> Unit) {
        val packet: GetFollowRequestsPacket = this.factory.makePacket(PacketType.GET_FOLLOW_REQUESTS) as GetFollowRequestsPacket
        packet.payload = GetFollowRequestsPacket.GetFollowRequestsPayload(this)
        sendToServer(packet, this, destination,  callback)
    }
}

The class is simplified just so that it can be easier to read. I'm sure that all of you can see a pattern in this code, but it's beyond my knowledge to know how to make it more structured.

Do not worry about the functionality, the focus of this question is about improving the structure.

The factory code

class PacketFactory : Serializable {

    @Throws(Exception::class)
    fun makePacket(type: PacketType): Packet {
        return when (type) {
            PacketType.REGISTRATION -> RegistrationPacket()
            PacketType.UPLOAD_IMAGE -> UploadImagePacket()
            PacketType.LIST_USER_IDS -> ListUsersPacket()
            PacketType.FOLLOW_USER -> FollowUserPacket()
            PacketType.GET_FOLLOW_REQUESTS -> GetFollowRequestsPacket()
            else -> throw Exception("Type is not provided for the factory!")
        }
    }

}

The sendToServer function

fun sendToServer(
        payload: Packet,
        sender: Client,
        receiver: Server,
        callback: ((data: Any?) -> Unit)? = null
): Unit {
    // Code that sends payload to receiver and then invokes the callback
}

Answer 1

First of all, a client-server relationship is kind of a 1:1 relation. That means that the client should probably have a connection to a server (or, if abstracted, a server) to talk with. Currently you are using a server parameter, but I would argue that this should be a field. This will also free one parameter of every method. Furthermore, currently it seems allowed to call just any server from the client class. First register and then call a totally different server.

Furthermore, if such a generic & relatively static parameter is present then it should probably be the first parameter. Currently the location of the server parameter seems to change per function, and that makes reading the functions harder.

The dispatchFollowEvent seems to be used to "follow a user", but that's only clear to me after reading the code. Similarly we have an upload method that seemingly uploads an image. Why not just followUser or uploadImage? It's a client, we expect it to "dispatch" and that it creates an "event" at the server.

I'm not sure why you would use a factory in above code. The makePacket code is really just an intermediate to the specific packets that you need anyway. So now you go: "packet specific method -> packet enum -> packet specific method". Sure you can do that, but why should you?

A more grievous thing is that your factory method seems to create partial packets without a payload. That means that packets are initially in an invalid state. The whole idea is that you manufacture concrete products inside a factory.

Another minor issue is the send part of the sendToServer method. A send method generally doesn't receive any data back. If you have a callback handler, why not call it call instead? You can leave the server part out of it maybe, that a client calls a server seems kind of logical.

Unfortunately I have to leave it at this, not just because it is late, but also because I don't think I've seen enough of your code / architecture to refactor the given code. It's not terrible code or anything, but it seems rather over-engineered and especially the naming suffers from it.