1
\$\begingroup\$

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
}
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Welcome to code review. Right now this question is off-topic because the code for some of the functions are missing. Specifically we need to see the functions sendToServer(), the function facory.makePacket(), and probably the definitions of the class packet. Currently there is not enough code to review and provide good suggestions. \$\endgroup\$ – pacmaninbw Apr 26 '20 at 21:07
  • 4
    \$\begingroup\$ "The class is simplified just so that it can be easier to read." Please don't do that. The devil is in the details; hiding these details causes the likely hood of an incorrect answer to increase. \$\endgroup\$ – Peilonrayz Apr 26 '20 at 22:37
  • 1
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Apr 27 '20 at 8:11
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you! I agree with everything you said! I won't refactor this code because its a university project and the project is over, but I will remember your words. My initial question still remains unanswered though. My question is "How can I make the client code more structured since I can see. a pattern which in the end of the day is just copy and pasting". I thought about. making a function dispatch and then pass the type and. figure out then what to do. In any case, thanks for your. answer, you helped me a lot in realizing what things are wrong and what things are not needed. \$\endgroup\$ – Dimitris Filippou May 22 '20 at 13:58
  • \$\begingroup\$ At least removing the server parameter will get you less copy paste. You can also remove this from that call, and assume this. For the other parts you first have to refactor the factory code I think; as indicated, I don't think I have enough info to propose a way of handling that. Glad if this was of help to you :) \$\endgroup\$ – Maarten Bodewes May 22 '20 at 22:24

Not the answer you're looking for? Browse other questions tagged or ask your own question.