In this post, we created a shopping application which is available on console and refactored it. It has only one class which has only one public and many private functions. Each function looks simple but the class can do anything for the application. In addition to that, it’s not easy to write test for it. Let’s improve that points in this post.
Go to the last post if you haven’t read it.
You can download the complete source code here
The final code is in src/shopping-app/ver5
Decide how to make it testable
The main public function looks like this below and there is no seam to inject mock object.
import * as readSync from "readline-sync";
...
export class Shop {
...
public run() {
console.log("Welcome to special shop. This is what you can do.");
this.displayCommandList();
while (true) {
const commandString = readSync.question("Input command: ");
const args = commandString.split(" ");
this.executeCommand(args);
}
}
...
}readSync is imported on the top in this file and it’s not possible to replace it with mock which means that we can’t input test data automatically in our test. To be precise, it may be possible to input test data via readSync.question function but we shouldn’t rely on the implementation because its implementation may change in the future. There are two directions to make this class testable. From overall functionality to small unit or the other way around.
- Overall to Small unit
- Pros: A test tests multiple things, which means whole functionalities, and it can be useful when refactoring. We can safely refactor code by having tests.
- Cons: Tests needs to be removed and written again when refactoring is done because tests written before refactoring are kind of integration test.
- Small unit to Overall
- Pros: We don’t have to write tests multiple times.
- Cons: We need to run the application each time when we refactor in order to confirm if the refactoring is correct.
It may depends on the class which way to choose but I want to choose Small unit to Overall direction in this example because the function’s output is console output and it’s hard to test due to calling console.log multiple times. I may choose Overall to Small unit if the main public function returns number or object which we can easily inspect the data.
Extract logic into a function in a class
First of all, we need to have another class to extract logic. Let’s consider what objects exist there and what role do they have. Following is my thought.
- Shop: Where Business logic exists. Handling user input and delegate the actual process to another class
- Shopping cart: Manage number of items and calculate total prices
- Item: Manage item name and price
- Coin: Manage coin type
- User: User input
addItemToCart
Let’s see the current code of addItemToCart first. We need to add specified item to shopping cart, therefore we need to move the logic to ShoppingCart class.
export class Shop {
...
private addItemToCart(itemName: string, numberOfItems: number) {
if (!(Object.values(Item) as string[]).includes(itemName)) {
console.log(`${itemName} doesn't exist.`);
return;
}
const currentNumber = this.shoppingCart.get(itemName) || 0;
this.shoppingCart.set(itemName, currentNumber + numberOfItems);
}
...
}The logic to check if the specified item exists is not the ShoppingCart class’s role. Let’s leave it as it is.
export class ShoppingCart {
private items = new Map<ItemName, number>();
public addItem(itemName: ItemName, numberOfItems: number): void {
if (numberOfItems <= 0) {
throw new Error("numberOfItems must be 1 or bigger number");
}
const currentNumber = this.items.get(itemName) || 0;
this.items.set(itemName, currentNumber + numberOfItems);
}
}I moved the logic but I changed the argument data type to enum. In addition to that, I fixed a bug that negative value was acceptable before even if it is a function to add item. I want to write a test for this function but it’s currently not possible because it’s void function. We will add getList function to get the current item list because we want to replace showItemsInCart function.
showItemsInCart
Let take a look at the function again.
export class Shop {
...
private showItemsInCart() {
const items = Array.from(this.shoppingCart.entries())
.reduce((acc, [key, value]) => acc + `${key}: ${value}\n`, "")
const totalNumber = Array.from(this.shoppingCart.values())
.reduce((acc, cur) => acc + cur, 0);
let totalPrice = this.calculateTotalPrice();
const message = items + `\ntotal number: ${totalNumber}\n`
+ `total price: ${totalPrice}`;
console.log(message);
}
private calculateTotalPrice() {
let totalPrice = 0;
this.shoppingCart.forEach((value, key) => {
switch (key) {
case ItemName.Apple:
totalPrice += Price.Apple * value;
break;
case ItemName.Water:
totalPrice += Price.Water * value;
break;
case ItemName.Coffee:
totalPrice += Price.Coffee * value;
break;
default:
console.error(`Undefined item exists in shopping cart [${key}]`);
}
});
return totalPrice;
}
...
}The logic for items, totalNumber and totalPrice can be moved to ShoppingCart class but not for the message because showing the result on the console is not the role of ShoppingCart class. Another thing we can improve here is calculateTotalPrice function. switch-case is used in the forEach loop to calculate the total price but it uses enum value here, like Price.Apple. If we want to add new item we need to add additional case even though the formula is the same for all. Item name and price should be defined once and shouldn’t be changed, so we can define it in following way.
export interface Item {
name: string;
price: number;
}
export class ItemHolder {
private static items: Item[] = [
{ name: ItemName.Apple, price: 110 },
{ name: ItemName.Coffee, price: 150 },
{ name: ItemName.Water, price: 90 },
];
public static getItemOf(name: string): Item {
const result = this.items.find((value) => value.name === name);
if (!result) {
throw Error(`Specified item name is undefined [${name}]`);
}
return result;
}
}Via getItemOf function we can get the price everywhere. The return data type is Item because we may add another properties in the future.
We can move the logic from showItemsInCart to ShoppingCart class and improve the logic for totalPrice by using this static function.
export class ShoppingCart {
private items = new Map<ItemName, number>();
public get totalItemNumber(): number {
return Array.from(this.items.values())
.reduce((acc, cur) => acc + cur, 0);
}
public get totalPrice(): number {
let result = 0;
this.items.forEach((numberOfItems, key) => {
const item = ItemHolder.getItemOf(key);
result += item.price * numberOfItems;
});
return result;
}
public getList(): ItemCount[] {
return Array.from(this.items.entries())
.map(([name, numberOfItems]) => {
return { name, numberOfItems }
});
}
}Writing tests
We can finally write tests for ShoppingCart class by using getList function. Important point for writing unit test is to focus on one thing in one test. Don’t test two or more things in the same test because we can’t know at first glance what the problem is when it fails.
describe("ShoppingCart", () => {
let cart: ShoppingCart;
beforeEach(() => {
cart = new ShoppingCart();
});
describe("addItem", () => {
it("should add an item", () => {
cart.addItem(ItemName.Apple, 2);
const result = cart.getList();
const expected = {
name: ItemName.Apple,
numberOfItems: 2,
};
expect(result).to.deep.include(expected);
});
it("should sum the number of items for the same item", () => {
cart.addItem(ItemName.Apple, 2);
cart.addItem(ItemName.Apple, 3);
const result = cart.getList();
const expected = {
name: ItemName.Apple,
numberOfItems: 5,
};
expect(result).to.deep.include(expected);
});
it("should add the different item", () => {
cart.addItem(ItemName.Apple, 2);
cart.addItem(ItemName.Water, 3);
const result = cart.getList();
const expected = [
{
name: ItemName.Apple,
numberOfItems: 2,
},
{
name: ItemName.Water,
numberOfItems: 3,
}
];
expect(result).to.deep.members(expected);
});
});
describe("getList", () => {
it("should return empty array when no data exist", () => {
const result = cart.getList();
expect(result).to.be.lengthOf(0);
});
it("should include name and numberOfItems", () => {
cart.addItem(ItemName.Apple, 4);
const result = cart.getList();
expect(result[0]).to.have.keys("name", "numberOfItems");
});
});
describe("totalPrice", () => {
it("should sum the price", () => {
cart.addItem(ItemName.Apple, 1);
cart.addItem(ItemName.Water, 1);
expect(cart.totalPrice).to.equal(200)
});
});
describe("totalItemNumber", () => {
it("should sum the number", () => {
cart.addItem(ItemName.Apple, 2);
cart.addItem(ItemName.Water, 1);
cart.addItem(ItemName.Coffee, 3);
expect(cart.totalItemNumber).to.equal(6)
});
});
});This example may not be best example because two functions are used in a test, for example addItem and getList. But it’s good enough to write the test. If you don’t want to call two functions in a test there is work around. It’s straightforward. The steps are
- Change the access modifier of the target variable from private to protected
- Create a subclass of the target class
- Define a public getter/function to expose the protected variable
- Use the public getter/function in a test
export class ShoppingCart {
protected items = new Map<ItemName, number>();
}
export class ExtendedShoppingCart extends ShoppingCart{
public get exposedItems(): Map<ItemName, number> {
return this.items;
}
}We can directly play with items variable in our test in this way by using ExtendedShoppingCart. It’s up to you which solution to apply. If getList function is not necessary in the application and we want to write test the void function we need to apply this.
Replacing function
I skip how to move the logic for removeItem function but its way is basically the same. From here, we need to replace the original function defined in Shop class with the new function in ShoppingCart class. Since addItemToCart function is void we can’t check whether we replaced the function correctly. To check the result we need to replace both addItemToCart and showItemsInCart at once. We need to use ShoppingCart class but its name is already used so let’s name it shoppingCartClass for now. The code looks like this.
export class Shop {
private shoppingCart = new Map<string, number>();
private shoppingCartClass = new ShoppingCart();
...
private addItemToCart(itemName: string, numberOfItems: number) {
if (!(Object.values(ItemName) as string[]).includes(itemName)) {
console.log(`${itemName} doesn't exist.`);
return;
}
this.shoppingCartClass.addItem(itemName as ItemName, numberOfItems);
}
private showItemsInCart() {
const items = this.shoppingCartClass.getList()
.reduce((acc, cur) => acc + `${cur.name}: ${cur.numberOfItems}\n`, "");
const message = items
+ `\ntotal number: ${this.shoppingCartClass.totalItemNumber}`
+ `\ntotal price: ${this.shoppingCartClass.totalPrice}`;
console.log(message);
}
}I didn’t remove shoppingCart variable. If I remove it the build fails because I didn’t replace other functions. Let’s run the app and confirm if I didn’t break anything.
Input command: add water 1
Input command: add apple 2
Input command: add coffee 3
Input command: cart
water: 1
apple: 2
coffee: 3
total number: 6
total price: 760It looks fine. Next step is removeItemFromCart. This is very easy.
export class Shop {
private removeItemFromCart(itemName: string, numberOfItems: number) {
this.shoppingCartClass.removeItem(itemName as ItemName,numberOfItems);
}
}Let’s run the app. It works fine.
Input command: add water 5
Input command: remove water 3
Input command: cart
water: 2
total number: 2
total price: 180
Input command: remove water 5
Input command: cart
total number: 0
total price: 0Last function to replace is pay function. The change is very small. We can remove calculateTotalPrice function and shoppingCart variable because it’s replaced with shoppingCartClass variable. I add comment lines to say where to change.
export class Shop {
private pay(amountOfMoney: string) {
if (amountOfMoney.slice(-1) !== "0") {
console.error("Ones place digit must not 0.");
return;
}
// Before
// const totalPrice = this.calculateTotalPrice();
// const change = parseInt(amountOfMoney, 10) - totalPrice;
// After
const change = parseInt(amountOfMoney, 10) - this.shoppingCartClass.totalPrice;
const coinList = new Map<string, number>([
["1000", 0],
["500", 0],
["100", 0],
["50", 0],
["10", 0],
]);
calculateNumberOfCoins();
showNumberOfCoins();
console.log(`change: ${change}`);
// Before
// this.shoppingCart.clear();
// After
this.shoppingCartClass.clear();
}
}The result looks like this. shoppingCartClass didn’t have clear function but it’s necessary here, so I added it. It just call clear function of Map class.
# Add items
Input command: add apple 2
Input command: add water 2
Input command: add coffee 1
Input command: cart
apple: 2
water: 2
coffee: 1
total number: 5
total price: 550
# Payment
Input command: pay 1000
100: 4
50: 1
change: 450
# Confirm if the all items are removed
Input command: cart
total number: 0
total price: 0Since the variable name shoppingCartClass looks strange we should rename it to shoppingCart then commit the change.
Improve displayItemList and ItemHolder
We can still improve Shop class. displayItemList uses ItemName enum but when we add item we need to add lines here too. Let’s see the current implementation first.
export class Shop {
private displayItemList() {
let itemList = `${ItemName.Apple}, ${Price.Apple}\n`;
itemList += `${ItemName.Water}, ${Price.Water}\n`;
itemList += `${ItemName.Coffee}, ${Price.Coffee}\n`;
console.log(itemList);
}
}We should use ItemHolder class instead of enum but ItemHolder class gave Object without freezing it which give a client capability to change the value! It means that the data is not capsulized correctly. Let’s use Object.freeze function to prevent from accidental value change.
export class ItemHolder {
private static items: Item[] = [
Object.freeze({ name: ItemName.Apple, price: 110 }),
Object.freeze({ name: ItemName.Coffee, price: 150 }),
Object.freeze({ name: ItemName.Water, price: 90 }),
];
public static getItemOf(name: string): Item {
const result = this.items.find((value) => value.name === name);
if (!result) {
throw Error(`Specified item name is undefined [${name}]`);
}
return result;
}
public static get list(): Item[] {
return this.items;
}
}
export class Shop {
...
private displayItemList() {
const itemList = ItemHolder.list
.reduce((acc, cur) => acc + `${cur.name}, ${cur.price}\n`, "");
console.log(itemList);
}
}Now, client can’t change the name and price. We don’t have to add lines to displayItemList function when we add items.
Further improvement
The important thing I want to tell is that we should replace functions one by one. Small step makes our refactoring easier because we can easily find the mistake when it breaks something. If we make big change at once we need to look for the root cause in the whole change which takes us longer time. When we finish one refactoring we should commit the change. There may be lots of commit entries before pushing it to remote server but we can put them together by git rebase.
Comments