CODEWITHSUNDEEP
javaoopdesign-patterns
Artemkller545
Artemkller545

Reputation: 999

Java prevent method duplication for different purposes

Hey I have these two methods:

private void sendMessage(String message) {
    ArrayList<Client> waiters = this.getPlayers().getWaitingRoom();
    for (Client c : waiters) {
        c.sendMessage(message);
    }
}

second method:

private void sendMessage(String message) {
    ArrayList<Client> players = this.getPlayers().getGame();
    for (Client c : players) {
        c.sendMessage(message);
    }
}

These methods are useful because I need somehow to send a message to the clients that are in the waiting room and the clients that are in game, the clients who are in game should not hear the messages that I send for the ones in the waiting room, same for opposite.

I came up with this solution to make it into one method but I feel that's its a very poor solution:

private void sendMessage(String message, int type) {
    ArrayList<Client> clients = (type == 0) ? this.getPlayers().getWaitingRoom() :
        this.getPlayers().getGame();
    for (Client c :clients) {
        c.sendMessage(message);
    }
}

This works, but I wondered if there is a proper solution for this, more object oriented. But since it is using the same type (ArrayList), I got a bit confused & lost.

Any ideas of a better design for this method?

Upvotes: 0

Views: 76

Answers (3)

Marco13
Marco13

Reputation: 54709

The first, obvious generalization would be to declare

private void sendMessage(String message, Iterable<? extends Client> clients) {
    for (Client c : clients) {
        c.sendMessage(message);
    }
}

That allows calling 

sendMessage("Message", this.getPlayers().getWaitingRoom());
sendMessage("Message", this.getPlayers().getGame());

BTW: Though the explicit declaration of ArrayList is usually not necessary. They could return List<Client>....

EDIT: Answering the question "Why Iterable...":

Iterable is the "smallest" interface that is sufficient for the task that you want to do in this method. You ONLY want to iterate over all elements. You don't need an ArrayList. You don't need a List. You don't even need a Collection. You ONLY need something that is Iterable.

The advantage is that this method may be called with many different arguments:

Set<Client> set = ...
List<Client> list = ...
Queue<Client> queue = ...
sendMessage("Message", set); // Works
sendMessage("Message", list); // Works
sendMessage("Message", queue ); // Works

Set<SomeClassExtendingClient> setWithSpecialClients = ...
sendMessage("Message", setWithSpecialClients); // Works as well...

If you declared the method to receive an ArrayList<Client>, you could not pass in a LinkedList<Client>, or an ArrayList<SpecialClient>. With Iterable<? extends Client>, you have the greatest possible flexibility.

Upvotes: 4

Someone
Someone

Reputation: 551

Java enums are designed in an object orientated way. Using an enum would make your code type-safe.

public enum Room {
    GAME, LOBBY;
}

private void sendMessage(String message, Room room) {
    ArrayList<Client> clients = (room == Room.LOBBY) ? this.getPlayers().getWaitingRoom() :
        this.getPlayers().getGame();
    for (Client c :clients) {
        c.sendMessage(message);
    }
}

Note: If you were to have more than two possibilities, you would use a switch statement.

Edit: The other answers would be the preferred solution; this is just another option.

Upvotes: 0

user3237736
user3237736

Reputation: 867

You are defining the wrong input for the method. The method's task is to send a message to a list of clients. It should neither care nor know about the deeper meaning of those clients. So what it should look like is this:

private void sendMessageToClients(String message, ArrayList<Client> clients) {
    for (Client c :clients) {
        c.sendMessage(message);
    }
}

The task to determine whether you choose the waiting room or the ingame players is to be done by the caller of the method.

Upvotes: 1

Related Questions