This is 3rd post in Become a better and decent programmer series.
In the first post, I explained how to extract logic in a big function into small functions. The goal was to have small and readable functions.
In the second post, I explained how to introduce unit test and move the functions into another class. The scale was bigger scope than the first post because we needed to consider the relationships between the classes.
This post explains how to reduce/remove switch-case clause for keeping complexity and make it more testable.
You can download the complete source code here
The final code is in src/shopping-app/ver6
Consider the role of Shop class
First of all, let’s check what function Shop class has.
export class Shop {
private shoppingCart = new ShoppingCart();
public run() {}
private displayCommandList() {}
private executeCommand(args: string[]) {
switch (args[0]) {
case Command.Command:
this.displayCommandList();
break;
case Command.List:
...
}
private displayItemList() {}
private addItemToCart(itemName: string, numberOfItems: number) {}
private removeItemFromCart(itemName: string, numberOfItems: number) {}
private showItemsInCart() {}
private pay(amountOfMoney: string) {}
}It looks there’s no problem with these functions. However, consider a case when we need to add a new function. We need to add additional private function in this class and add a condition in switch-case clause. The more functions we have, the more complex the code is. We want to keep the complexity even we have lots of functions.
Let’s consider the class role to make this class clean. In real, Shop is a place where a customer can buy something. It is just a place where there are shopping carts, customers, cashers, products, and so on. And shop itself doesn’t have will or thought. In programming world, it means that Shop class shouldn’t know about how the processes work. The role of Shop class is to have Objects and call those functions defined the Objects according to the action. It’s like a manager or boss.
Okay, let’s improve the code.
Create a command interface and implement it
To delegate the actions, we can simply create a class and move existing functions there but we still have to add additional case clause into switch-case in executeCommand function in this case. Instead of this way, I create command interface which has only execute function. Since Command is already used by enum I rename the enum to CommandName.
export enum CommandName {
List = "list",
Add = "add",
Remove = "remove",
Cart = "cart",
Pay = "pay",
Command = "command",
Exit = "exit",
}
export interface Command {
execute(): void;
}Then, create classes which implement the interface. This is a replacement for displayCommandList function.
export class DisplayCommand implements Command {
public execute(): void {
const commandList = `${CommandName.Command}: list the available commands\n`
+ `${CommandName.List}: show the item list and prices\n`
+ `${CommandName.Add}: add the item to shopping cart. Format is 'add <item name> <number>'\n`
+ `${CommandName.Remove}: remove the item from the shopping cart. Format is 'remove <item name> <number>'\n`
+ `${CommandName.Cart}: list items in shopping cart\n`
+ `${CommandName.Pay}: pay the cost and receive change. Format is 'pay <money>'\n`;
console.log(commandList);
}
}To remove switch-case clause in executeCommand we need to have a function to get one of classes which implements Command interface.
export class CommandHolder {
private commands: Map<CommandName, Command>;
constructor() {
this.commands = new Map([
[CommandName.Command, new DisplayCommand()],
]);
}
public getCommand(name: CommandName): Command {
const result = this.commands.get(name);
if (!result) {
throw new Error(`Specified command is undefined. [${name}]`);
}
return result;
};
}We are going to add Command classes into this class and client side can get corresponding Command class according to the command name. This function will be replaced with switch-case clause. Using switch-case clause increases code complexity but this way keeps it regardless of the number of commands.
Replace command function
Since we implemented necessary class and function next step is to replace existing function with it. Let’s instantiate CommandHolder in Shop class. It may be better to pass the instance from constructor for the unit test but I don’t do it at the moment.
export class Shop {
private commandHolder = new CommandHolder();
private executeCommand(args: string[]) {
switch (args[0]) {
case CommandName.Command:
// this.displayCommandList();
this.commandHolder.getCommand(args[0]).execute();
break;
case CommandName.List:
...
}
}Let’s run the application to confirm if the replacement doesn’t break anything but I don’t show the result here. Anyway, the basic refactoring steps is following.
- Move the target logic into a xxxCommand class
- Replace the existing function with the Command
- Run the application and confirm if it doesn’t break anything
- Repeat the process above
Implement commands with/without arguments
Since we already implemented one function it is easy to implement others! Do you think so? Some functions require arguments and some don’t. We need to somehow handle these difference but we don’t want to have switch-case to handle it. How can we resolve this? executeCommand requires an argument like this.
private executeCommand(args: string[]) {}We need to somehow pass this arguments to execute function. Let’s add the same argument to execute function.
export interface Command {
execute(args?: string[]): void;
}We can use this arguments like args[1] but it is not readable because we don’t know what value the second index has. For this reason, I want to convert the string[] argument to specified object required by a function before using it in actual logic. Whenever Command function requires arguments we need to convert string[] to a object. Therefore, I created an abstract class for it.
export interface CommandArgs { }
export abstract class ArgsCommandBase<T extends CommandArgs> implements Command {
public execute(args: string[]): void {
const commandArgs = this.convert(args);
this.process(commandArgs);
}
protected abstract process(args: T): void;
protected abstract convert(args: string[]): T;
}CommandArgs has no variable because each command requires different arguments. The execute function in this class always requires args without ? keyword. Command class which requires arguments needs to be extended from this class and implement work logic in process function and mapping logic in convert function. Following code is an example.
export class ListCommand implements Command {
public execute(): void {
const itemList = ItemHolder.list
.reduce((acc, cur) => acc + `${cur.name}, ${cur.price}\n`, "");
console.log(itemList);
}
}
interface AddCommandArgs extends CommandArgs {
itemName: string;
numberOfItems: number;
}
export class AddCommand extends ArgsCommandBase<AddCommandArgs> {
constructor(private shoppingCart: ShoppingCart) {
super();
}
protected process(args: AddCommandArgs): void {
if (!(Object.values(ItemName) as string[]).includes(args.itemName)) {
console.log(`${args.itemName} doesn't exist.`);
return;
}
this.shoppingCart.addItem(args.itemName as ItemName, args.numberOfItems);
console.log(this.shoppingCart)
}
protected convert(args: string[]): AddCommandArgs {
if (args.length < 2) {
throw new Error("Add command requires 2 arguments.");
}
return {
itemName: args[0],
numberOfItems: parseInt(args[1], 10),
};
}
}Implement Command interface directly if a command doesn’t require any arguments. Otherwise, extends ArgsCommandBase class. Now, it’s clear which index has what value but it’s better to add parameter validation logic here in real. This time I decided to use constructor injection for shoppingCart variable. At first, I considered to include it in the argument but I didn’t because it is always the same object. It doesn’t make sense to pass it as an argument. If we pass it from constructor the class can always use the same object. CommandHolder looks like this below.
export class CommandHolder {
private commands: Map<CommandName, Command>;
constructor(shoppingCart: ShoppingCart) {
this.commands = new Map();
this.commands.set(CommandName.Command, new DisplayCommand());
this.commands.set(CommandName.List, new ListCommand());
this.commands.set(CommandName.Add, new AddCommand(shoppingCart));
}
}I changed the implementation a little. I use set function instead of instantiation in constructor because it shows an error. Let’s apply this to Shop class.
export class Shop {
private shoppingCart = new ShoppingCart();
private commandHolder = new CommandHolder(this.shoppingCart);
private executeCommand(args: string[]) {
switch (args[0]) {
case CommandName.Command:
case CommandName.List:
case CommandName.Add:
const commandArgs = args.slice(1);
this.commandHolder.getCommand(args[0]).execute(commandArgs);
break;
case CommandName.Remove:
...
}
}
}Since first index args[0] is command name we need to exclude it when passing the arguments to execute function. That’s why I used args.slice(1). This is shallow copy for the array. After this refactoring, run the application as usual.
Input command: list
apple, 110
coffee, 150
water, 90
Input command: add apple 2
Input command: add water 3
Input command: cart
apple: 2
water: 3
total number: 5
total price: 490
Input command: After adding some items, cart command still shows information because it passes the same object to the CommandHolder class. Since the steps for the other functions are the same as this one I skip the explanation.
The Shop class eventually looks like this.
export class Shop {
private shoppingCart = new ShoppingCart();
private commandHolder = new CommandHolder(this.shoppingCart);
public run() {
console.log("Welcome to special shop. This is what you can do.");
this.commandHolder.getCommand(CommandName.Command).execute();
while (true) {
const commandString = readSync.question("Input command: ");
const args = commandString.split(" ");
this.executeCommand(args);
}
}
private executeCommand(args: string[]) {
try {
const commandArgs = args.slice(1);
this.commandHolder.getCommand(args[0]).execute(commandArgs);
} catch (e) {
console.error(e);
}
}
}
export class CommandHolder {
private commands: Map<CommandName | string, Command>;
public getCommand(name: CommandName | string): Command {
...
};
}It is clear and very easy to read! I changed the key datatype of CommandHolder.commands because this.commandHolder.getCommand(args[0]) in executeCommand showed error. If I implement this.commandHolder.getCommand(args[0] as CommandName) it can solve the problem but I didn’t. Using as keyword reduces the advantage of Typescript because it tells the compiler not to check the actual datatype. It’s better to avoid using as keyword if possible.
Write Unit test
Now, we have many command classes. Since it’s isolated from Shop class we can write test for them. The test code for AddCommand class looks like following.
describe("AddCommand", () => {
let shoppingCart: ShoppingCart;
let command: AddCommand;
let cartStub: sinon.SinonStub;
beforeEach(() => {
shoppingCart = new ShoppingCart();
cartStub = sinon.stub(shoppingCart, "addItem");
command = new AddCommand(shoppingCart);
});
afterEach(() => {
cartStub.restore();
})
describe("execute", () => {
[
{ itemName: "apple", count: 1 },
{ itemName: "water", count: 2 },
{ itemName: "coffee", count: 3 },
].forEach((data) => {
it(`should call addItem when specifying ${data.itemName}`, () => {
command.execute([data.itemName, data.count.toString()]);
expect(cartStub.calledWith(data.itemName as any, data.count)).to.be.true;
});
});
it("should not call addItem when specifying item which doesn't exist", () => {
command.execute(["Table", "1"]);
expect(cartStub.notCalled).to.be.true;
});
it("should throw an error when arg length is 1", () => {
const result = () => command.execute(["Hey"]);
expect(result).to.throw("requires 2 arguments.")
});
});
});It’s simple enough. I put the 3 cases into one test definition together because it is basically the same test. If putting them together into one test requires writing condition clause like if-else or switch-case it should be written separately. I wrote test only for AddCommand class. Currently we can’t write test for some classes which don’t require shoppingCart because they just call console.log. If we want to write test for these classes we need to do further refactoring.
Further improvement
In this article, I explained how to remove switch-case condition. I introduced command pattern and it is useful when a service requires many commands. Those command classes are testable and small enough but we still have some issues. Some classes are not testable.
Comments