Is this Command Pattern design ok?

AbuseMePlz · 26-03-2013 11:34pm #1

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 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?

RobotController class:

private Robot r;
public void driveRobot(){
	    
		do{
			processCommands();
			
		}while(true);
	}

public String processCommands(){
	
	    Command r = new RCommand(r);
	    Command l = new LCommand(r);
	     Command f = new FCommand(r);
		
	    CommandInvoker c = new CommandInvoker();
		
		// 1. Get user input
                // Loop through each command
			switch(command){
	            	case 'F': 
	            		c.execute(f);
	            		break;
	            	case 'R': 
	            		c.execute(r);
    				    break;
	            	case 'L': 
	            		c.execute(l);
			            break;
				}	
			}
		}
		//Display new position of robot
	}

Main class:

public class Main {
	public static void main(String[] args) {
		
		RobotController r = new RobotController();		
		r.driveRobot();
	}
}

Giblet · 27-03-2013 8:58am

Surely command invoker can use polymorphism rather than a switch.
Instead of switching on a variable named command, create a command which can be invoked.

Colonel Panic · 27-03-2013 9:15am

Does CommandInvoker need to be a separate class? Does is have any additional members or even more than just an execute method? As Giblets says, if all commands have a Command interface, and you create them based on inputs, then all you need to is call Execute without some proxy goes nowhere, does nothing class.

AbuseMePlz · 27-03-2013 10:13am

Thanks for your replies.

I am trying to follow the Command Pattern I have been reading up on. All of the examples have a separate CommandInvoker class. Do you think it's redundant?

Right now, the inputs are finite (left, right, forward) but at some point in time, there could be a need to add 'Back'. I want my solution to be extendable with little effort.

If a user inputs FFRFLF, I then loop through each character (command) and call each command's execute() method.

AbuseMePlz · 27-03-2013 10:14am

Giblet wrote: »

Surely command invoker can use polymorphism rather than a switch.

Do you suggest adding all my commands to a list and then run through the list calling execute()?

What do you mean by:

Giblet wrote: »

Instead of switching on a variable named command, create a command which can be invoked.

AbuseMePlz · 27-03-2013 1:13pm

How does this look?

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();
			}
		}
	}

Giblet · 27-03-2013 2:31pm

Ah the for-switch makes an appearance.

That's just doing

new TurnRightCommand(robot).execute();
new TurnLeftCommand(robot).execute();
new ForwardCommand(robot).execute();

What you should do, is that based on the input, execute a command.
Or, if you are deferring the command, create the command, and store it to be executed later.

So you should have an input handler which delegates which command should be created based on the input.

I would start here. Rather than storing the inputs from the user, use the inputs to create commands directly?

String cmds = get user input from console

AbuseMePlz · 27-03-2013 2:37pm

Giblet wrote: »
What you should do, is that based on the input, execute a command.
Or, if you are deferring the command, create the command, and store it to be executed later.

So you should have an input handler which delegates which command should be created based on the input.

I would start here. Rather than storing the inputs from the user, use the inputs to create commands directly?
String cmds = get user input from console

cmds gets set to a string of commands, e.g. FFRF. I need to parse this string into individual commands. Can you elaborate what you mean by 'which command should be created based on the input'?

AbuseMePlz · 27-03-2013 3:23pm

Giblet wrote: »

That's just doing

new TurnRightCommand(robot).execute();
new TurnLeftCommand(robot).execute();
new ForwardCommand(robot).execute();

I get what you mean but how to I separate the user input into individual commands?

Procasinator · 27-03-2013 6:15pm

You could use a Map instead, with the key being the character and the value being the respective command.

croo · 28-03-2013 9:59am

OP ask yourself where are the 4 main components of the Command Pattern?
Command, Receiver, Invoker & Client.

The Receiver would be your robot.

Each command the Robot can accept would have its own Command class that would call the relevant Robot method(s).

The Invoker would manage and execute the commands... in this case I would see two methods one to add commands to a list and another execute the lists of commands.

Finally there is the Client which would create instances of the Robot & Commands and parse the string of commands -passed in as a args to the main perhaps?- however you capture the command string it would parse this and for each command call the Invoker and add a command to its List of Commands. Once finished parsing you can call the Invoker's execute method to execute all the commands.

AbuseMePlz · 28-03-2013 10:06am

croo wrote: »

OP ask yourself where are the 4 main components of the Command Pattern?
Command, Receiver, Invoker & Client.

The Receiver would be your robot.

Each command the Robot can accept would have its own Command class that would call the relevant Robot method(s).

The Invoker would manage and execute the commands... in this case I would see two methods one to add commands to a list and another execute the lists of commands.

Finally there is the Client which would create instances of the Robot & Commands and parse the string of commands -passed in as a args to the main perhaps?- however you capture the command string it would parse this and for each command call the Invoker and add a command to its List of Commands. Once finished parsing you can call the Invoker's execute method to execute all the commands.

My solution is similar to this.

croo · 28-03-2013 11:32am

AbuseMePlz wrote: »

My solution is similar to this.

From the initial code you posted there is not a complete picture so it might be.

I would ask... what do you think is the benefit of the Command Pattern in this Scenario. Usual reasons for using this pattern might be to create a macro recording or perhaps undo functionality. What is the purpose here?

Why not just parse the command list and call the robot commands directly ?

AbuseMePlz · 28-03-2013 11:35am

croo wrote: »

From the initial code you posted there is not a complete picture so it might be.

I would ask... what do you think is the benefit of the Command Pattern in this Scenario. Usual reasons for using this pattern might be to create a macro recording or perhaps undo functionality. What is the purpose here?

Why not just parse the command list and call the robot commands directly ?

I wanted the ability to easily add more commands. I thought this would have given me flexibility.

Fluffy88 · 28-03-2013 2:40pm

The command pattern is perfect for what you want to do.
The issue here is your not implementing it fully correctly.

		// read user input
		String commands = "FFRLFRF";

		for (int i = 0; i < commands.length(); i++) {
			char userCmd = commands.charAt(i);

			// all commands implement the interface Command
			Command c = null;
			switch (userCmd) {
			case 'F':
				c = new ForwardCommand();
				break;
			case 'R':
				c = new RightCommand();
				break;
			case 'L':
				c = new LeftCommand();
				break;
			}
			
			// Command interface defines the execute method
			c.execute();

Each implementation of the Command interface defines how to do it's movement/action in the execute() method.

Then to add a new Command, create the implementation of the command and add another case to the switch statement for it.
Ideally for this kind of pattern, you would place the Mapping of the input characters (i.e. F, R, L...) to Commands into a XML or properties file and instead of the switch statement read the mapping from the file, that way you don't need to change the processCommands() method when you want to add a new Command.

Another alternative to the XML file would be, as suggested already, use an actual Java Map to store the Mapping of Commands to Characters. You could then in the Command implementation have the command place it's own Mapping into the Map, therefore to add a new Command you would only need to create the Command class and you would be done.

croo · 28-03-2013 3:33pm

AbuseMePlz wrote: »

I wanted the ability to easily add more commands. I thought this would have given me flexibility.

I meant that as a rhetorical question to help you see what you might be missing and why!

Is this Command Pattern design ok?

Comments