Multithreaded Message Game

Question

Use cases Messaging app between two users.

GameMain is for multithreading application refers to SendMessageWrokers.java

Only 2 user need to be defined and message app needs to finalize after a restriced amount of message count.

GameMain.java

package org.nb;

import java.util.List;
import java.util.concurrent.*;

public class GameMain{

    public final static int MESSAGE_LIMIT = 10;
    Player initiator = new Player(0,"Initiator",true);
    Player player = new Player(1,"Receiver");
    SendMessageWorker messageWorker;

    public GameMain(){
        Player[] players =new Player[2];
        players[0] = initiator;
        players[1] = player;
    }

    public static void main(String[] args) {
        GameMain gameMain = new GameMain();
        gameMain.startGame();
    }

    public void startGame(){

        Runtime.getRuntime().addShutdownHook(new Thread(() -> System.out.println("Shutdown Hook is running, Application Terminating")));

        Integer counter = 0;

        BlockingQueue<Runnable> blockingQueue = new ArrayBlockingQueue<>(10);

        ExecutorService executorService = new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, blockingQueue);

        while (true) {
            counter++;
            messageWorker =  new SendMessageWorker(counter.toString(),"Sending a new message ",initiator,player);
            executorService.submit(messageWorker);
            if (counter >= MESSAGE_LIMIT) {
                break;
            }
        }

        executorService.shutdown();
        try {
            executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
        } catch (InterruptedException e) {

        }

        Conversation conversation = initiator.getConversationHashMap(player.getId());

        List<Message> messageList = conversation.getConversation();

        for(Message message : messageList){
            System.out.println("Message : " + message.getContent() + " - Date :" + message.getDate() +
                    "- Sender " + message.getSender().getName() +  "- Receiver " + message.getReceiver().getName() );
        }

        System.out.println("Application Terminating ...");
    }

}

Conversation.java

package org.nb;

import java.util.ArrayList;

public class Conversation {

    private Player participants[];

    private ArrayList<Message> conversation = new ArrayList<>();

    public Conversation(Player player1 , Player player2){
        participants = new Player[2];
        this.participants[0] = player1;
        this.participants[1] = player2;
    }

    void addMessage(Message message){
        conversation.add(message);
    }

    public ArrayList<Message> getConversation() {
        return conversation;
    }

}

Message.java

package org.nb;

import java.util.Date;

public class Message {

    private String content;
    private Player sender;
    private Player receiver;
    private Date date;
    private Boolean ack = false;

    public Message(String content, Player player, Player toPlayer) {
        this.content = content;
        this.sender = player;
        this.receiver = toPlayer;
        this.date = new Date();
    }

    public Message(String content, Player player, Player toPlayer,Boolean ack) {
        this.content = content;
        this.sender = player;
        this.receiver = toPlayer;
        this.date = new Date();
        this.ack = ack;
    }

    public String getContent() {
        return content;
    }

    public Player getSender() {
        return sender;
    }

    public Player getReceiver() {
        return receiver;
    }

    public Date getDate() {
        return date;
    }

}

Player.java

package org.nb;

import java.util.HashMap;

public class Player {

    private int id;
    private String name;
    private boolean initiator = false;

    private HashMap<Integer,Conversation> conversationHashMap = new HashMap<>();

    public Player(int id,String name, boolean initiator){
        this.id=id;
        this.name=name;
        this.initiator=initiator;
    }

    public Player(int id, String name){
        this.id=id;
        this.name=name;
    }

    public int getId() {
        return id;
    }

    public String getName() {
        return name;
    }

    public boolean isInitiator() {
        return initiator;
    }

    public Conversation getConversationHashMap(int id) {
        return conversationHashMap.get(id);
    }

    public void setConversationHashMap(Integer playerId, Conversation  conversation) {
        this.conversationHashMap.put(playerId, conversation);
    }

}

SendWorkerMessage.java

package org.nb;

public class SendMessageWorker implements Runnable {

    private String name = "SendMessageTask";

    String content;
    Player initiator;
    Player receiver;

    private String counter;

    public SendMessageWorker (String counter,String content,Player initiator, Player receiver) {
        this.counter = counter;
        this.content = content;
        this.initiator = initiator;
        this.receiver = receiver;
    }

    @Override
    public void run() {

            if (initiator.isInitiator() == false) {
                System.out.println("Only initiator can send message");
                return;
            }

            Conversation conversation = initiator.getConversationHashMap(receiver.getId());
            if (conversation == null) {
                conversation = new Conversation(initiator, receiver);
            }

            Message message = new Message(content, initiator, receiver);
            conversation.addMessage(message);

            Message messageAck = new Message(content.concat(String.valueOf("_" + counter)), receiver, initiator, true);
            conversation.addMessage(messageAck);

            initiator.setConversationHashMap(receiver.getId(), conversation);
            receiver.setConversationHashMap(initiator.getId(), conversation);

    }

}

Answer 1

Design

The design is OK; it seems to use data classes and some classes that perform operations.

In true OO fashion, I would expect the player to send the message. For instance, you could do Player.sendMessageTo(int otherPlayerID, String message). You could initialize the player with a MessageHandler that does the actual sending of the message. The player then can keep a list of conversations with other player(s). That way you can hide much of the technical details from the "game". Similarly, you can have a Player.receiveAcknowledgement that is triggered by the same MessageHandler.

Code review

GameMain

public final static int MESSAGE_LIMIT = 10;
Player initiator...

I'd always separate the constant values and the fields using an empty line.

Player initiator = new Player(0,"Initiator",true);

This is why you don't use boolean parameters: it is completely unclear what true does here when browsing through the code. Instead, use an enum PlayerType with values INITIATOR and RECEIVER. Note that you could now do away with the name - unless you want to have multiple receivers.

A less intrusive refactor would be to use a constant INITIATOR = true to be used. This has the disadvantage that other developers overlook the constant though (for Cipher.getInstance in Java I keep seeing Cipher.getInstance(1, ...) instead of Cipher.getInstance(Cipher.ENCRYPT_MODE)).

Consider maybe not numbering players, but using the name itself as ID (using a HashMap<String, Player> nameToPlayer, for instance). Generally you want to avoid having fields that have a similar function.

Integer counter = 0;

Always prefer basic types: int counter = 0. Auto-boxing will take care of the rest.

BlockingQueue<Runnable> blockingQueue = new ArrayBlockingQueue<>(10);

ExecutorService executorService = new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, blockingQueue);

Why 10, 1, 1 and 0L? You need to use well named constant values here.

messageWorker =  new SendMessageWorker(counter.toString(),"Sending a new message ",initiator,player);

I'd try and not stringify before it is necessary. Just use the counter as parameter. Don't forget your code style, and add spaces after the commas. Note that you can use Integer.toString(int) to convert; no need to box it.

if (counter >= MESSAGE_LIMIT) {
    break;
}

I've got nothing against while (true) { ... if (condition) break ...} in principle but why not just a for loop here?

Conversation conversation = initiator.getConversationHashMap(player.getId());

As reader I would expect to get a HashMap here, not a specific object. Furthermore, it is not clear to me what a "conversation hash map" is.

List<Message> messageList = conversation.getConversation();

Hmm, that's slightly weird, get a conversation from a conversation. getMessages maybe?

Conversation

I would expect a list of players. Try and avoid arrays, the are not very OO.

Then you would get:

public Conversation(Player ... players){
    participants = List.of(players);
}

and note that the actual constructor call is not even changed.

public ArrayList<Message> getConversation() {
    return conversation;
}

Try and avoid to expose the state of an object instance, you're failing to encapsulate the field. Return either an iterator or return Collections.unmodifiableList(conversation).

Message

private Boolean ack = false;

ack will automatically be assigned false. ack is never read, and it is again an object instance rather than boolean.

Date should preferably not be used; try and use Instant.now() instead (and possibly also include a clock as parameter, for testing purposes).

public Conversation getConversationHashMap(int id) {
    return conversationHashMap.get(id);
}

You probably refactored this, but forgot to change the name of the method. getConversationFromHashMap would be better, but it would expose the inner workings of the class. getConversationWithPlayer(int id) is probably best.

Note that because Conversation is not immutable, the returned value again exposes state.

SendMessageWorker

if (initiator.isInitiator() == false) {
    System.out.println("Only initiator can send message");
    return;
}

Never keep running with invalid state. Bad programmer. Down (the application)!

If you should not get into such a state: throw new IllegalStateException("Only initiator can send message").

if (conversation == null) {
    conversation = new Conversation(initiator, receiver);
}

Ugh, don't do this. Either use an empty conversation and intialize in the object, or - if that's too expensive - lazily instantiate a conversation.

Note that this is resolved if you let the user keep track of their own conversation(s). A good design will lead you into fewer traps.