Oriented-object approach on how to control different classes cleanly

Question

I'm currently creating a client/server application (using Java) and I want to make it fully controllable by an input stream (currently the console). At the moment only one class have to be controlled, but I want my code to stay easy to develop.

I found a solution but I think I can do better, so that's why I post this message: I want your advices on my method now and how I could improve it or how should it better be.

Here's my solution:

I created an interface CommandControllable like:

import java.util.ArrayList;

public interface CommandControllable
{
    public void execute(Command command);
    public void addCommandManager(CommandManager commandManager);
    public ArrayList<CommandManager> getCommandManagers();
}

A class Command like

public class Command
{
    protected String request;

    public Command(String request)
    {
        this.request = request;
    }

    public String ToString()
    {
        return this.request;
    }
}

A class CommandManager

import java.io.InputStream;
import java.util.Scanner;

public class CommandManager implements Runnable
{
    protected Scanner reader;
    protected CommandControllable toControl;
    protected boolean running = false;

    public CommandManager(InputStream stream)
    {
        this.reader = new Scanner(stream);
    }

    @Override
    public void run()
    {
        this.running = true;
        while(this.running)
            {
                if(this.reader.hasNextLine())
                    {
                        Command c = new Command(this.reader.nextLine());
                        this.toControl.execute(c);
                    }
            }
    }

    public void setControllable(CommandControllable toControl)
    {
        this.toControl = toControl;
    }

    public void stop()
    {
        this.running = false;
    }
}

If I want a class to be controllable I write something like:

public class Server implements CommandControllable
{
    protected ArrayList<CommandManager> commandManagers = new ArrayList<CommandManager>();

    public Server()
    {

    }

    public void stop()
    {
        System.out.println("Server stopped");
    }

    @Override
    public synchronized void execute(Command command)
    {
        switch(command.ToString())
        {
            case "quit":
                stop();
                break;
            default:
                System.out.println("Unrecognized command");
                break;
        }
    }

    @Override
    public void addCommandManager(CommandManager commandManager)
    {
        commandManager.setControllable(this);
        this.commandManagers.add(commandManager);
    }

    @Override
    public ArrayList<CommandManager> getCommandManagers()
    {
        return this.commandManagers;
    }
}

And

public static void main(String[] args)
{
    Server s = new Server();
    CommandManager manager = new CommandManager(System.in);
    s.addCommandManager(manager)

    Thread t_manager = new Thread(manager);
    t_manager.start();
}

Any advices on the current solution and how to improve it?

Answer 1

CommandManager referes to a CommandControllable, and CommandControllable refers to a set of CommandManagers. Who controls who? Such loops in dependency graph should be avoided.

Then, think if the Actor model can be applied in your case.

Answer 2

Just four quick notes:

1. public ArrayList<CommandManager> getCommandManagers();
   

   should be

   public List<CommandManager> getCommandManagers();
   

   See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

2. public String ToString()
   

   is rather confusing, since there is an Object.toString and it's not clear from the name that which string does it return. I'd call it getRequest().

3. The CommandManager is not thread safe. Accessing of the running and toControl fields should be synchonized.

   If multiple threads access the same mutable state variable without appropriate synchronization, your program is broken. There are three ways to fix it:

   • Don't share the state variable across threads;
   • Make the state variable immutable; or
   • Use synchronization whenever accessing the state variable.

   From Java Concurrency in Practice by Brian Goetz.

   Using an AtomicBoolean for the reader and an AtomicReference for toControl would solve it. (You might use volatile fields here too.)

4. Streams and scanners usually should be closed somewhere. Although you are currently reading from System.in other clients might get streams for TCP connections or files which should be closed. It should be clear that whose (callee or caller) responsibility is the closing and when/where should it happen.