I hadn’t been decent programmer for a long time because I hadn’t written unit test. Decent programmer should at least be able to write unit test. If there are unit test in the code other developers can easily refactor the code because unit test tells them what’s wrong with their modifications.
I didn’t know how good code looks like and how to learn it. After I read books I became better programmer than before. Reading book helps but I will guide you to become better programmer with this series.
The better programmer you become, the more widely you can consider about the applications/services you develop. There are lots of things that programmer should consider but following is very simple indicator. I don’t say this is all for developer.
- Being able to make the app work
- Being able to give proper name to variable/function/class
- Being able to separate function/method properly
- Being able to write unit test
- Being able to separate class properly
This post will explain how to step up to 3 from step 2 which means you will be able to write a class which has simple and readable functions. In the later posts, you will be able to step up to step 4 and 5.
You can download the complete source code here
Requirement
This is the first requirement for the application that we develop. This is shopping application which is available on console.
- console application
- commands
command: list the available commandslist: show the item list and pricesadd: add the item to shopping cart. Format isadd <item name> <number>, e.g.add apple 2. If specified item name doesn’t exist it shows an error message.remove: remove the item from the shopping cart. Format isremove <item name> <number>, e.g.remove apple 2. If the number specified number is bigger than the number in shopping cart, the item is completely removed. If the specified item name doesn’t exist in the shopping cart it shows an error message.cart: list item names and number of each item and number of total items in shopping cart and show total pricespay: pay the cost and receive change. Format ispay <money>, e.g.pay 1100which means you pay 1100. If the ones place digit is not 0, it shows an error message. Cart should be empty after the payment.
- virtual coins are used for the payment
- coin type
- 10
- 50
- 100
- 500
- 1000
- items
- apple: 110
- water: 90
- coffee: 150
If you think you are programming beginner it’s better to develop this application first before going forward. Then, you will find where you are and what you can improve.
The code that beginners tend to write
This is I guess the code that beginners tend to write. They may even not create a class and may start writing logic directly in a main file (app.ts).
shopping-app/ver1
// app.ts import { Shop } from "./Shop"; let shop = new Shop(); shop.run(); // Shop.ts import * as readSync from "readline-sync"; export class Shop { public run() { console.log("Welcome to special shop. This is what you can do."); let commandList = "command: list the available commandsn"; commandList += "list: show the item list and pricesn"; commandList += "add: add the item to shopping cart. Format is `add <item name> <number>`n"; commandList += "remove: remove the item from the shopping cart. Format is `remove <item name> <number>`n"; commandList += "cart: list items in shopping cartn"; commandList += "pay: pay the cost and receive change. Format is `pay <money>`n"; console.log(commandList); const shoppingCart = new Map<string, number>(); while (true) { const commandString = readSync.question("Input command: "); const args = commandString.split(" "); if (args[0] === "command") { console.log(commandList); } else if (args[0] === "list") { let itemList = "apple, 110n"; itemList += "water, 90n"; itemList += "coffee, 150n"; console.log(itemList); } else if (args[0] === "add") { if (args[1] === "apple") { const currentNumber = shoppingCart.get("apple") || 0; const addedNumber = parseInt(args[2], 10); shoppingCart.set("apple", currentNumber + addedNumber); } else if (args[1] === "water") { const currentNumber = shoppingCart.get("water") || 0; const addedNumber = parseInt(args[2], 10); shoppingCart.set("water", currentNumber + addedNumber); } else if (args[1] === "coffee") { const currentNumber = shoppingCart.get("coffee") || 0; const addedNumber = parseInt(args[2], 10); shoppingCart.set("coffee", currentNumber + addedNumber); } else { console.log(`${args[1]} doesn't exist.`); } } else if (args[0] === "remove") { const currentNumber = shoppingCart.get(args[1]) || 0; const removeNumber = parseInt(args[2], 10); const result = currentNumber - removeNumber; if (result <= 0) { shoppingCart.delete(args[1]); } else { shoppingCart.set(args[1], result); } } else if (args[0] === "cart") { let cart = ""; let totalNumber = 0; let totalPrice = 0; shoppingCart.forEach((value, key) => { cart += `${key}: ${value}n`; totalNumber += value; if (key === "apple") { totalPrice += 110 * value; } else if (key === "water") { totalPrice += 90 * value; } else if (key === "coffee") { totalPrice += 150 * value; } }); const message = cart + `ntotal number: ${totalNumber}n` + `total price: ${totalPrice}`; console.log(message); } else if (args[0] === "pay") { if (args[1].slice(-1) === "0") { let totalPrice = 0; shoppingCart.forEach((value, key) => { if (key === "apple") { totalPrice += 110 * value; } else if (key === "water") { totalPrice += 90 * value; } else if (key === "coffee") { totalPrice += 150 * value; } }); const change = parseInt(args[1], 10) - totalPrice; const coinList = new Map<string, number>([ ["1000", 0], ["500", 0], ["100", 0], ["50", 0], ["10", 0], ]); let rest = change; coinList.forEach((value, key) => { if (rest > 0) { const numberOfCoins = Math.floor(rest / parseInt(key, 10)); if (numberOfCoins > 0) { rest = change % parseInt(key, 10); coinList.set(key, numberOfCoins); } } }); coinList.forEach((value, key) => { if (value > 0) { console.log(`${key}: ${value}`); } }); console.log(`change: ${change}`); shoppingCart.clear(); } else { console.error("Ones place digit must not 0."); } } else if (args[0] === "exit") { console.log("Thank you for your shopping.") break; } else { console.error("Undefined command."); } } } }
What do you think when you look at this code? Beginners may not give proper name to the variables but let’s skip the step. It’s hard to understand what’s going on at first glance and we need to take a deep look at each clause.
Anyway, let’s run the application first.
> node ./dist/ver1/app.js
Welcome to special shop. This is what you can do.
command: list the available commands
list: show the item list and prices
add: add the item to shopping cart. Format is `add <item name> <number>`
remove: remove the item from the shopping cart. Format is `remove <item name> <number>`
cart: list items in shopping cart
pay: pay the cost and receive change. Format is `pay <money>`
Input command: add apple 2
Input command: add water 1
Input command: add coffee 1
Input command: cart
apple: 2
water: 1
coffee: 1
total number: 4
total price: 460
Input command: pay 1000
500: 1
10: 4
change: 540
Input command: exit
Thank you for your shopping.
It seems to work well but how about error case?
Input command: pay 100
100: 1
change: 100
Input command: add apple 1
Input command: pay 50
change: -60
Input command: Undefined command.
Input command:
Oops! Its behavior looks strange. The code didn’t handle the following cases.
- when a customer pays with empty cart
- when a customer gives less money than the total price
The error cases are actually not described in the requirement. However, it is not rare when error cases are not written in the requirement. Better programmers should take care error cases but it is not easy for beginners to recognize it. A reviewer should check if the error cases are handled correctly. These problems can easily happen when a function is big because we can’t focus on one thing.
Let’s create a issue list for this program in order to improve it one by one.
- The command name used in command list is hard coded.
- when changing the command name it can different from actual command name
- The same item name and price are used in different places.
- when item name or price changes we have to change them all.
- if writing unit test it tests several things at once
- logic is not separated
- not readable
- hard to remember logic
- hard to focus on one thing and can forget to implement one of specifications. Especially error handling.
- Too many if-else clauses
- hard to follow the logic because it’s hard find corresponding brace
- the more conditional statements are used, the more complex the code is
Introduce Enum for constant values
First improvement is to use constant value. We can define them as private members but let’s use enum for this example because I don’t want to define a lot of private members in a class. I think having lots of members in a class is one of code smell. It’s a sign that the class becomes big. Following code is the after improvement.
shopping-app/ver2
enum Command { List = "list", Add = "add", Remove = "remove", Cart = "cart", Pay = "pay", Command = "command", Exit = "exit", } enum Item { Apple = "apple", Water = "water", Coffee = "coffee", } enum price { Apple = 110, Water = 90, Coffee = 150, } export class Shop { public run() { console.log("Welcome to special shop. This is what you can do."); let commandList = `${Command.Command}: list the available commandsn`; commandList += `${Command.List}: show the item list and pricesn`; commandList += `${Command.Add}: add the item to shopping cart. Format is 'add <item name> <number>'n`; commandList += `${Command.Remove}: remove the item from the shopping cart. Format is 'remove <item name> <number>'n`; commandList += `${Command.Cart}: list items in shopping cartn`; commandList += `${Command.Pay}: pay the cost and receive change. Format is 'pay <money>'n`; console.log(commandList); ... if (args[0] === Command.Command) { console.log(commandList); } else if (args[0] === Command.List) { let itemList = `${Item.Apple}, ${price.Apple}n`; itemList += `${Item.Water}, ${price.Water}n`; itemList += `${Item.Coffee}, ${price.Coffee}n`; console.log(itemList); } else if (args[0] === Command.Add) { ... } else if (args[0] === Command.Cart) { let cart = ""; let totalNumber = 0; let totalPrice = 0; shoppingCart.forEach((value, key) => { cart += `${key}: ${value}n`; totalNumber += value; if (key === Item.Apple) { totalPrice += price.Apple * value; } else if (key === Item.Water) { totalPrice += price.Water * value; } else if (key === Item.Coffee) { totalPrice += price.Coffee * value; } });
I didn’t show all codes but you can understand what I did. There is no magic number there. Once we introduce enum value instead of hard-coded value we can easily change the value without consideration. Just change the value in enum definition.
It’s important not to use magic number in code because other developer cannot understand what it is. When looking at this code they cannot understand what 110 is unless they know about the specification. 110 may be item ID or number of stock.
} else if (args[0] === "list") { let itemList = "apple, 110n"; itemList += "water, 90n"; itemList += "coffee, 150n"; console.log(itemList); }
Extract logic into a function from run function
Extracting logic into a function is used most often for refactoring. It’s very easy but very good start point to refactor. Extracting logic into a function means that we give a name to the logic. The name should be clear enough to explain what the function does. Use this technique to divide the logic if you have complex and long logic. You may want to add some comments for it but giving a proper name to the logic can remove the comments. It’s difficult to give a proper name to a function if the function does more than 2 things. Consider to move logic from the function in this case.
The code is in shopping-app/ver3
Display command list
Let’s extract logic to show command list first.
export class Shop { public run() { console.log("Welcome to special shop. This is what you can do."); this.displayCommandList(); const shoppingCart = new Map<string, number>(); while (true) { const commandString = readSync.question("Input command: "); const args = commandString.split(" "); if (args[0] === Command.Command) { this.displayCommandList(); ... } } } private displayCommandList() { let commandList = `${Command.Command}: list the available commandsn`; commandList += `${Command.List}: show the item list and pricesn`; commandList += `${Command.Add}: add the item to shopping cart. Format is 'add <item name> <number>'n`; commandList += `${Command.Remove}: remove the item from the shopping cart. Format is 'remove <item name> <number>'n`; commandList += `${Command.Cart}: list items in shopping cartn`; commandList += `${Command.Pay}: pay the cost and receive change. Format is 'pay <money>'n`; console.log(commandList); } }
Now displayCommandList() function has its own logic and commandList variable is encapsulated. Before this refactoring, there was a risk that the variable can be changed after showing command list.
Command execution
Next step is to move command execution logic. The code looks like this below.
export class Shop { public run() { console.log("Welcome to special shop. This is what you can do."); this.displayCommandList(); const shoppingCart = new Map<string, number>(); while (true) { const commandString = readSync.question("Input command: "); const args = commandString.split(" "); this.executeCommand(); } } private executeCommand(){ ... } }
By extracting the logic, main function looks clear. Since the logic in executeCommand is too long I didn’t write the code here but there are actually many errors because shoppingCart and args variables are used in the function which are not declared in the function. Let’s change the implementation. shoppingCart variable can be private member because only one instance is available and its data has to remain until pay command is called. On the other hand, args should be in while loop since a user runs command multiple times. Therefore, it needs to be passed to executeCommand function as an argument.
export class Shop { private shoppingCart = new Map<string, number>(); 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); } } private executeCommand(args: string[]) { if (args[0] === Command.Command) { this.displayCommandList(); } else if (args[0] === Command.List) { ... } ... }
There is still an error in the function because exit command calls break without loop clause.
} else if (args[0] === Command.Exit) { console.log("Thank you for your shopping.") break; } else {
We need to somehow change it. Return a result code or call process.exit(). Let’s call process.exit() for now because it’s easier.
} else if (args[0] === Command.Exit) { console.log("Thank you for your shopping.") process.exit(); } else {
Declare function for each command
Shop class has two simple functions but executeCommand is still complex. It’s really horrible function. If you write this type of function on your work you will learn many things from my blog.
First step to improve this function is to extract logic into a function. executeCommand looks like this.
The code is in shopping-app/ver4.
export class Shop { ... private executeCommand(args: string[]) { if (args[0] === Command.Command) { this.displayCommandList(); } else if (args[0] === Command.List) { this.displayItemList(); } else if (args[0] === Command.Add) { this.addItemToCart(args[1], parseInt(args[2], 10)); } else if (args[0] === Command.Remove) { this.removeItemFromCart(args[1], parseInt(args[2], 10)) } else if (args[0] === Command.Cart) { this.showItemsInCart(); } else if (args[0] === Command.Pay) { this.pay(args[1]); } else if (args[0] === Command.Exit) { console.log("Thank you for your shopping.") process.exit(); } else { console.error("Undefined command."); } } ... }
It looks much better than before but I don’t like writing the same variable names multiple times! It looks redundant Let’s use switch-case instead.
export class Shop { ... private executeCommand(args: string[]) { switch (args[0]) { case Command.Command: this.displayCommandList(); break; case Command.List: this.displayItemList(); break; case Command.Add: this.addItemToCart(args[1], parseInt(args[2], 10)); break; case Command.Remove: this.removeItemFromCart(args[1], parseInt(args[2], 10)) break; case Command.Cart: this.showItemsInCart(); break; case Command.Pay: this.pay(args[1]); break; case Command.Exit: console.log("Thank you for your shopping.") process.exit(); default: console.error("Undefined command."); } } ... }
It’s personal preference though I think it looks nicer than using if-else. Now, it’s clear what executeCommand does.
Refactoring each command function
Let’s check whether the other functions are simple enough or not.
displayItemList
This is simple enough.
export class Shop { ... private displayItemList() { let itemList = `${Item.Apple}, ${Price.Apple}n`; itemList += `${Item.Water}, ${Price.Water}n`; itemList += `${Item.Coffee}, ${Price.Coffee}n`; console.log(itemList); } ... }
addItemToCart
export class Shop { ... private addItemToCart(itemName: string, numberOfItems: number) { if (itemName === Item.Apple) { const currentNumber = this.shoppingCart.get(Item.Apple) || 0; this.shoppingCart.set(Item.Apple, currentNumber + numberOfItems); } else if (itemName === Item.Water) { const currentNumber = this.shoppingCart.get(Item.Water) || 0; this.shoppingCart.set(Item.Water, currentNumber + numberOfItems); } else if (itemName === Item.Coffee) { const currentNumber = this.shoppingCart.get(Item.Coffee) || 0; this.shoppingCart.set(Item.Coffee, currentNumber + numberOfItems); } else { console.log(`${itemName} doesn't exist.`); } } ... }
I just extracted the logic into a function therefore there is still room for improvement because the logic in each if-else clause is the same.
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); } ... }
I often write if(!isConditionFulfilled){} when it can return in the clause in order to reduce number of clauses. Even if we need to add additional if-else for sub-sequent process in the future the number of indents is still one. It’s more readable, right?
removeItemFromCart
It looks simple.
export class Shop { ... private removeItemFromCart(itemName: string, numberOfItems: number) { const currentNumber = this.shoppingCart.get(itemName) || 0; const result = currentNumber - numberOfItems; if (result <= 0) { this.shoppingCart.delete(itemName); } else { this.shoppingCart.set(itemName, result); } } ... }
showItemsInCart
export class Shop { ... private showItemsInCart() { let cart = ""; let totalNumber = 0; let totalPrice = 0; this.shoppingCart.forEach((value, key) => { cart += `${key}: ${value}n`; totalNumber += value; if (key === Item.Apple) { totalPrice += Price.Apple * value; } else if (key === Item.Water) { totalPrice += Price.Water * value; } else if (key === Item.Coffee) { totalPrice += Price.Coffee * value; } }); const message = cart + `ntotal number: ${totalNumber}n` + `total price: ${totalPrice}`; console.log(message); } ... }
It looks good and simple at the moment but I changed the implementation a bit.
export class Shop { ... private showItemsInCart() { const items = Array.from(this.shoppingCart.entries()) .reduce((acc, [key, value]) => `${key}: ${value}n`, "") const totalNumber = Array.from(this.shoppingCart.values()) .reduce((acc, cur) => acc + cur, 0); let totalPrice = 0; this.shoppingCart.forEach((value, key) => { switch (key) { case Item.Apple: totalPrice += Price.Apple * value; break; case Item.Water: totalPrice += Price.Water * value; break; case Item.Coffee: totalPrice += Price.Coffee * value; break; default: console.error(`Undefined item exists in shopping cart [${key}]`); } }); const message = items + `ntotal number: ${totalNumber}n` + `total price: ${totalPrice}`; console.log(message); } ... }
Main point for this refactoring is to separate the concern. Before this refactoring, all calculations were done in a forEach function. It means we needed to remember all of them at the same time. If we separate the concern we can focus on only one thing and can reduce number of mistakes. Since this function is small this refactoring doesn’t make much sense but separating concern helps to get mistake away. Some developers say that this is less performance because of multiple loop inside of the process like forEach/reduce but it doesn’t lead to the problem in most cases. If we find performance problem in the future it is easier to find the root cause and fix the it.
pay
The last function is pay function which is longest and most complex.
export class Shop { ... private pay(amountOfMoney: string) { if (amountOfMoney.slice(-1) === "0") { let totalPrice = 0; this.shoppingCart.forEach((value, key) => { if (key === Item.Apple) { totalPrice += Price.Apple * value; } else if (key === Item.Water) { totalPrice += Price.Water * value; } else if (key === Item.Coffee) { totalPrice += Price.Coffee * value; } }); const change = parseInt(amountOfMoney, 10) - totalPrice; const coinList = new Map<string, number>([ ["1000", 0], ["500", 0], ["100", 0], ["50", 0], ["10", 0], ]); let rest = change; coinList.forEach((value, key) => { if (rest > 0) { const numberOfCoins = Math.floor(rest / parseInt(key, 10)); if (numberOfCoins > 0) { rest = change % parseInt(key, 10); coinList.set(key, numberOfCoins); } } }); coinList.forEach((value, key) => { if (value > 0) { console.log(`${key}: ${value}`); } }); console.log(`change: ${change}`); this.shoppingCart.clear(); } else { console.error("Ones place digit must not 0."); } } ... }
I don’t like having long statements in if-else clause because it’s hard to find corresponding curly brackets. This function has else clause but it has only one line in it. I want to write the statement first.
export class Shop { ... private pay(amountOfMoney: string) { if (amountOfMoney.slice(-1) !== "0") { console.error("Ones place digit must not 0."); return; } ... } ... }
Next point is calculation of total price. The logic is the same as showItemsInCart function. Let’s extract it into a function and call it.
export class Shop { ... private calculateTotalPrice() { let totalPrice = 0; this.shoppingCart.forEach((value, key) => { switch (key) { case Item.Apple: totalPrice += Price.Apple * value; break; case Item.Water: totalPrice += Price.Water * value; break; case Item.Coffee: totalPrice += Price.Coffee * value; break; default: console.error(`Undefined item exists in shopping cart [${key}]`); } }); return totalPrice; } private pay(amountOfMoney: string) { if (amountOfMoney.slice(-1) !== "0") { console.error("Ones place digit must not 0."); return; } const totalPrice = this.calculateTotalPrice(); ... } ... }
It could reduce number of lines in pay function. There are two logic which use coinList to calculate number of coins for change and to show the result. However, we need to read the logic to understand what it does because its logic doesn’t have name. Since it’s simple let’s extract it into a function inside the function.
export class Shop { ... private pay(amountOfMoney: string) { if (amountOfMoney.slice(-1) !== "0") { console.error("Ones place digit must not 0."); return; } const totalPrice = this.calculateTotalPrice(); const change = parseInt(amountOfMoney, 10) - totalPrice; const coinList = new Map<string, number>([ ["1000", 0], ["500", 0], ["100", 0], ["50", 0], ["10", 0], ]); calculateNumberOfCoins(); showNumberOfCoins(); console.log(`change: ${change}`); this.shoppingCart.clear(); function calculateNumberOfCoins() { let rest = change; coinList.forEach((value, key) => { if (rest > 0) { const numberOfCoins = Math.floor(rest / parseInt(key, 10)); if (numberOfCoins > 0) { rest = rest % parseInt(key, 10); coinList.set(key, numberOfCoins); } } }); } function showNumberOfCoins() { coinList.forEach((value, key) => { if (value > 0) { console.log(`${key}: ${value}`); } }); } } ... }
We can now easily understand what the function does. function in function is available in Typescript and it can access to const variables defined in the parent function. Other top level functions defined in the class can’t call the inner function.
Further improvement
We finished first refactoring. Shop class is much more readable than the first version but it has only one public function and many private functions. I think beginners who just learn Object Oriented Programming tend to implement like this.
Do you think we can write unit test for this class?
What is the role of the class? It does almost everything. When one of specifications changes we must change the class. There are many reasons to change the class and it violates to Single Responsibility Principle.
Let’s check how to improve this application further.
Comments