Opinions on my Command pattern design - is it extensible?

Question

I am implementing a program that takes commands from a user and based on those commands, drives a Robot on a grid. To implement this program I have used the Command pattern. I have used this pattern because, most importantly, I want to be able to add to the list of possible commands in the future.

The program takes a string of commands from the user, and executes those commands, e.g. FFRFLF (forward x 2, turn right, forward, turn left, forward).

Can anyone give me their opinion on the quality of my design?

Robot Controller class:

    public void control(){

do{
    process();  
}while(tru);
}

public String process(){

    // 1. Get user input
    String cmds = get user input from console

    // 2. Populate command list
    populateCommandList(cmds);

    // 3. Execute commands
    executeCommands();

       // 4. Display current location of robot
}

public void populateCommandList(String cmds) {

    Command r = new TurnRightCommand(robot);
    Command l = new TurnLeftCommand(robot);
    Command f = new ForwardCommand(robot);

    commands = new ArrayList<Command>();

        for(int i=0; i<cmds.length(); i++){

            char cmd = cmds.charAt(i);
            switch(cmd){
                case 'F': 
                    commands.add(f);
                    break;
                case 'R': 
                    commands.add(r);
                    break;
                case 'L': 
                    commands.add(l);
                    break;
            }   
        }   
}

private void executeCommands() {
    if(commands!=null && commands.size()>0){
        for(Command cmd : commands){
            cmd.execute();
        }
    }
}

Main class:

public class Main {
    public static void main(String[] args) {

        RobotController r = new RobotController();      
        r.control();
    }
}

Is this a good design? I need to know if the deign is good and that I can easily add new commands with minimal rework.

Answer 1

I think you've added an unnecessary layer of abstraction here.

To me, populateCommandList looks completely unnecessary. You are converting a list of commands in one format (character string) into another format (an array of Command objects). Then, presumably, executeCommands just executes all of those command objects in order. This wastes time and memory by creating a bunch of unnecessary objects.

Why not just have executeCommands take the string directly, and execute the appropriate command for each character that it encounters?

In other words, I would have something like this in executeCommands:

   for(int i=0; i<cmds.length(); i++){
       char cmd = cmds.charAt(i);
         switch(cmd){
            case 'F': 
                robot.move(FORWARD,1);
                break;
            case 'R': 
                robot.turn(RIGHT);
                break;
            case 'L': 
                robot.turn(LEFT);
                break;
        }
   }

Either way looks equally extensible to me. Either way you will have a switch statement somewhere that defines what commands are possible, and that is what you have to change in order to add a new command.