In the previous post, we cleaned up our code to a great extent, but there is still room for optimization. If you look at the processPayment and processRefund methods, you will notice the presence of repeated if conditions, which is a sign of code duplication:
function processPayment(paymentTransaction) {
if (paymentTransaction.method === "CREDIT_CARD") {
processCreditCardPayment(paymentTransaction);
} else if (paymentTransaction.method === "PAYPAL") {
processPayPalPayment(paymentTransaction);
} else if (paymentTransaction.method === "PLAN") {
processPlanPayment(paymentTransaction);
}
}
function processRefund(refundTransaction) {
if (refundTransaction.method === "CREDIT_CARD") {
processCreditCardRefund(refundTransaction);
} else if (refundTransaction.method === "PAYPAL") {
processPayPalRefund(refundTransaction);
} else if (refundTransaction.method === "PLAN") {
processPlanRefund(refundTransaction);
}
}As you can see, we are constantly checking the transaction method between the three values of CREDIT_CARD, PAYPAL, and PLAN. The main problem in this code is that our conditions are similar, but we perform different operations for each of these conditions, and these operations are not similar in any way. Note that I have written only console.log in the above methods (such as processPlanRefund or processCreditCardRefund or processCreditCardPayment etc.) to simplify the code:
function processCreditCardPayment(transaction) {
console.log(
"Processing credit card payment for amount: " + transaction.amount
);
}
function processCreditCardRefund(transaction) {
console.log(
"Processing credit card refund for amount: " + transaction.amount
);
}
function processPayPalPayment(transaction) {
console.log("Processing PayPal payment for amount: " + transaction.amount);
}
function processPayPalRefund(transaction) {
console.log("Processing PayPal refund for amount: " + transaction.amount);
}
function processPlanPayment(transaction) {
console.log("Processing plan payment for amount: " + transaction.amount);
}
function processPlanRefund(transaction) {
console.log("Processing plan refund for amount: " + transaction.amount);
}But in real applications, these functions will be responsible for processing transactions and perform banking operations. Therefore, it is impossible to merge these codes together, but we can make it cleaner in a special way. What method? We can reverse the logic of this condition! what does it mean?
Currently, our program first checks the transaction type in the processTransaction method to find out if the transaction is a Payment or Refund type:
function processTransaction(transaction) {
if (transactions.status !== "OPEN") {
showErrorMessage("Invalid transaction type!");
return;
}
if (transaction.type === "PAYMENT") {
processPayment(transaction);
} else if (transaction.type === "REFUND") {
processRefund(transaction);
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}As shown in the code above, we call one of the processPayment or processRefund methods based on the type of transaction. By reversing this condition, we can act like this: first, instead of checking the transaction type, we check the transaction method (CREDIT_CARD, PAYPAL, and PLAN) and then, in the second step, we check the transaction type. By doing this, we don't need to check the transaction method separately every time and cause code duplication. With this account, our first step to edit this code is to write separate functions to determine the transaction method:
function usesCreditCard(transaction) {
return transaction.method === "CREDIT_CARD";
}
function usesPayPal(transaction) {
return transaction.method === "PAYPAL";
}
function usesPlan(transaction) {
return transaction.method === "PLAN";
}As you can see, these three functions define what a transaction method is. By return a comparison like the code above, either true or false is returned to us, so we understand what the type of transaction is. The codes above are not technically a problem, but they are overwritten! Why? Because we can merge all three into a single function:
function usesTransactionMethod(transaction, method) {
return transaction.method === method;
}Our function has two parameters: the first parameter is the desired transaction and the second parameter is the method we want to compare! So we can compare the transaction method in any case and we don't need three separate functions. Now that we have this function, we can go back to the processTransaction function and edit it like this:
function processTransaction(transaction) {
if (!isOpen(transaction)) {
showErrorMessage("Invalid transaction type!");
return;
}
if (usesTransactionMethod(transaction, "CREDIT_CARD")) {
// ...
} else if (usesTransactionMethod(transaction, "PAYPAL")) {
// ...
} else if (usesTransactionMethod(transaction, "PLAN")) {
// ...
}
}Now we have three simple conditions, each of which checks the transaction method. From this part onwards, we have to decide what to do in each of these conditions. The simplest and best way is to have three separate methods for processing the above three types of transactions. I define these three methods as follows:
function processCreditCardTransaction(transaction) {}
function processPayPalTransaction(transaction) {}
function processPlanTransaction(transaction) {}As a challenge, I want you to write the content inside these functions based on the code we already have and then look at my answer. Naturally, the first step is to call these methods in processTransaction:
function processTransaction(transaction) {
if (!isOpen(transaction)) {
showErrorMessage("Invalid transaction type!");
return;
}
if (usesTransactionMethod(transaction, "CREDIT_CARD")) {
processCreditCardTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PAYPAL")) {
processPayPalTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PLAN")) {
processPlanTransaction(transaction);
}
}In the next step, we start writing the content of these functions:
function processCreditCardTransaction(transaction) {
if (isPayment(transaction)) {
processCreditCardPayment();
} else if (isRefund(transaction)) {
processCreditCardRefund();
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}
function processPayPalTransaction(transaction) {
if (isPayment(transaction)) {
processPayPalPayment();
} else if (isRefund(transaction)) {
processPayPalRefund();
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}
function processPlanTransaction(transaction) {
if (isPayment(transaction)) {
processPlanPayment();
} else if (isRefund(transaction)) {
processPlanRefund();
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}Obviously, to completely reverse the logic of the program, we have moved the if conditions for PAYMENT and REFUND into each of these methods. Although we still have code duplication and due to the general logic of the program, there is no way to completely remove it, but now the code is easier to read because we have distributed the if conditions among the programs. Currently, all the code in my file looks like this:
main();
function main() {
const transactions = [
{
id: "t1",
type: "PAYMENT",
status: "OPEN",
method: "CREDIT_CARD",
amount: "23.99",
},
{
id: "t2",
type: "PAYMENT",
status: "OPEN",
method: "PAYPAL",
amount: "100.43",
},
{
id: "t3",
type: "REFUND",
status: "OPEN",
method: "CREDIT_CARD",
amount: "10.99",
},
{
id: "t4",
type: "PAYMENT",
status: "CLOSED",
method: "PLAN",
amount: "15.99",
},
];
processTransactions(transactions);
}
function processTransactions(transactions) {
if (isEmpty(transactions)) {
showErrorMessage("No transactions provided!");
return;
}
for (const transaction of transactions) {
processTransaction(transaction);
}
}
function isEmpty(transactions) {
return !transactions || transactions.length === 0;
}
function showErrorMessage(message, item) {
console.log(message);
if (item) {
console.log(item);
}
}
function processTransaction(transaction) {
if (!isOpen(transaction)) {
showErrorMessage("Invalid transaction type!");
return;
}
if (usesTransactionMethod(transaction, "CREDIT_CARD")) {
processCreditCardTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PAYPAL")) {
processPayPalTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PLAN")) {
processPlanTransaction(transaction);
}
}
function isOpen(transaction) {
return transaction.status === "OPEN";
}
function usesTransactionMethod(transaction, method) {
return transaction.method === method;
}
function isPayment(transaction) {
return transaction.type === "PAYMENT";
}
function isRefund(transaction) {
return transaction.type === "REFUND";
}
function processCreditCardTransaction(transaction) {
if (isPayment(transaction)) {
processCreditCardPayment();
} else if (isRefund(transaction)) {
processCreditCardRefund();
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}
function processPayPalTransaction(transaction) {
if (isPayment(transaction)) {
processPayPalPayment();
} else if (isRefund(transaction)) {
processPayPalRefund();
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}
function processPlanTransaction(transaction) {
if (isPayment(transaction)) {
processPlanPayment();
} else if (isRefund(transaction)) {
processPlanRefund();
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}
function processCreditCardPayment(transaction) {
console.log(
"Processing credit card payment for amount: " + transaction.amount
);
}
function processCreditCardRefund(transaction) {
console.log(
"Processing credit card refund for amount: " + transaction.amount
);
}
function processPayPalPayment(transaction) {
console.log("Processing PayPal payment for amount: " + transaction.amount);
}
function processPayPalRefund(transaction) {
console.log("Processing PayPal refund for amount: " + transaction.amount);
}
function processPlanPayment(transaction) {
console.log("Processing plan payment for amount: " + transaction.amount);
}
function processPlanRefund(transaction) {
console.log("Processing plan refund for amount: " + transaction.amount);
}In my opinion, our codes have become much more readable.
Don't underestimate mistakes!
As I explained in the previous post, errors can be a very useful tool to make our code more readable. You must be asking how? A common mistake among developers is that they avoid throwing custom errors because throwing and handling errors removes extra ifs from our functions. So as a general rule, if something looks like an error, make it an error! The biggest mistake is to make it an if condition. For example, consider the following code:
if (!isEmail) {
return {code: 422, message: 'Invalid Input'};
}As you can see, here we check whether a data is an email and if this data is not really an email, we return an object that has the characteristics of code and message. After doing this, we should probably receive this object in another part of the program and then check its code to see if the user's input should be accepted or not! Simply put, we are creating a custom error using if conditions when we could simply use an Error in our own programming language:
if (!isEmail) {
const error = new Error('Invalid input');
error.code = 422;
throw error;
}This time instead of passing objects between different parts of the code, we simply throw an error. Although this code is written in JavaScript, all programming languages have a similar mechanism, which will ultimately be different in details.
We have several similar examples in the code we've worked on so far where the situation calls for using errors, but we haven't used them. An example of that is the processTransactions method:
function processTransactions(transactions) {
if (isEmpty(transactions)) {
showErrorMessage("No transactions provided!");
return;
}
for (const transaction of transactions) {
processTransaction(transaction);
}
}In this example, we have checked whether the transaction is empty (isEmpty) and if the transaction is really empty, we have displayed an error text to the user using the showErrorMessage method. In terms of code readability, error printing should not be part of a transaction's processing, the transaction processing should actually handle processing rather than printing program errors. The better way is to do this in the main method.
One of the possible solutions is to do something like the example I explained so that instead of printing an error, the processTransactions method returns an object that has the code and message properties:
function processTransactions(transactions) {
if (isEmpty(transactions)) {
return { code: 1, message: "No transaction provided!" }
}
for (const transaction of transactions) {
processTransaction(transaction);
}
}Now, in the main method, we can call the processTransactions method as follows:
function main() {
const transactions = [
{
id: "t1",
type: "PAYMENT",
status: "OPEN",
method: "CREDIT_CARD",
amount: "23.99",
},
{
id: "t2",
type: "PAYMENT",
status: "OPEN",
method: "PAYPAL",
amount: "100.43",
},
{
id: "t3",
type: "REFUND",
status: "OPEN",
method: "CREDIT_CARD",
amount: "10.99",
},
{
id: "t4",
type: "PAYMENT",
status: "CLOSED",
method: "PLAN",
amount: "15.99",
},
];
const result = processTransactions(transactions);
if (result.code === 1) {
showErrorMessage(result.message);
}
}This method is slightly better than our previous method because it removes the error message printing from the transaction processing method, but it is still bad because we have added an extra if condition to our code and instead of using the simple mechanism Let's use error, we have tried to make our own error mechanism! A better way, as explained, is to use errors:
function processTransactions(transactions) {
if (isEmpty(transactions)) {
const error = new Error('No transactions provided!');
error.code = 1;
throw error;
}
for (const transaction of transactions) {
processTransaction(transaction);
}
}Due to the nature of the throw statement and throwing errors in JavaScript, when we reach the throw error statement, the execution of the code is stopped and the for loop and the codes after it are no longer executed. Now we have to manage this error in a part of the program. I go to the main function together and say:
function main() {
const transactions = [
{
id: "t1",
type: "PAYMENT",
status: "OPEN",
method: "CREDIT_CARD",
amount: "23.99",
},
{
id: "t2",
type: "PAYMENT",
status: "OPEN",
method: "PAYPAL",
amount: "100.43",
},
{
id: "t3",
type: "REFUND",
status: "OPEN",
method: "CREDIT_CARD",
amount: "10.99",
},
{
id: "t4",
type: "PAYMENT",
status: "CLOSED",
method: "PLAN",
amount: "15.99",
},
];
try {
processTransactions(transactions);
} catch (error) {
showErrorMessage(error.message);
}
}As you can see I have used a try - catch block. These blocks work in such a way that the try section is executed first, and if we have an error, everything stops and we enter the catch section, which automatically receives that error. We have access to the error object in catch, so we can extract its message property and display it. This method is the correct and standard way of doing this.
We have two other sections in our code that deal with this problem. The first part is the processTransaction method:
function processTransaction(transaction) {
if (!isOpen(transaction)) {
showErrorMessage("Invalid transaction type!");
return;
}
if (usesTransactionMethod(transaction, "CREDIT_CARD")) {
processCreditCardTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PAYPAL")) {
processPayPalTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PLAN")) {
processPlanTransaction(transaction);
}
}As you can see, we have also used showErrorMessage in this section and we do not have a proper error. The second part also includes all methods of processCreditCardTransaction, processPayPalTransaction, and processPlanTransaction. All these methods have a similar structure and at the end they print a simple string instead of throwing an error. Let's review the processCreditCardTransaction method for example:
function processCreditCardTransaction(transaction) {
if (isPayment(transaction)) {
processCreditCardPayment();
} else if (isRefund(transaction)) {
processCreditCardRefund();
} else {
showErrorMessage("Invalid transaction type!", transaction);
}
}If the transaction is neither a payment nor a withdrawal of the customer's money, it means that the transaction is invalid and we have done this by printing a simple string. Of course, keep in mind that we also attach and print the transaction itself. The processPayPalTransaction and processPlanTransaction methods are exactly the same.
To correct these issues, we start from the processTransaction method:
function processTransaction(transaction) {
if (!isOpen(transaction)) {
const error = new Error('Invalid transaction type.');
throw error;
}
if (usesTransactionMethod(transaction, "CREDIT_CARD")) {
processCreditCardTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PAYPAL")) {
processPayPalTransaction(transaction);
} else if (usesTransactionMethod(transaction, "PLAN")) {
processPlanTransaction(transaction);
}
}Adding the code attribute to the Error object was just for example and I don't really need it, so I haven't done it in this section. All we have done is create a new error and then throw it. In the next step, we go to the processCreditCardTransaction method to correct it in the same way:
function processCreditCardTransaction(transaction) {
if (isPayment(transaction)) {
processCreditCardPayment();
} else if (isRefund(transaction)) {
processCreditCardRefund();
} else {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
}In this method, we must also have the transaction itself, so we have added it to the error object in the form of a property called item. In JavaScript, we can add any attribute with any name to an object even if it doesn't already exist, that's why we can add item like this. Finally, we throw the error. What do you think if we do this for other methods?
function processCreditCardTransaction(transaction) {
if (isPayment(transaction)) {
processCreditCardPayment();
} else if (isRefund(transaction)) {
processCreditCardRefund();
} else {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
}
function processPayPalTransaction(transaction) {
if (isPayment(transaction)) {
processPayPalPayment();
} else if (isRefund(transaction)) {
processPayPalRefund();
} else {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
}
function processPlanTransaction(transaction) {
if (isPayment(transaction)) {
processPlanPayment();
} else if (isRefund(transaction)) {
processPlanRefund();
} else {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
}As you can see, we have repeated our codes and this repetition of code is considered a defect in terms of writing clean code. What do you think is the solution? The best solution is to use a guard! how about That is, in the first step, we remove the entire else part of the above three methods:
function processCreditCardTransaction(transaction) {
if (isPayment(transaction)) {
processCreditCardPayment();
} else if (isRefund(transaction)) {
processCreditCardRefund();
} else {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
}
function processPayPalTransaction(transaction) {
if (isPayment(transaction)) {
processPayPalPayment();
} else if (isRefund(transaction)) {
processPayPalRefund();
} else {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
}
function processPlanTransaction(transaction) {
if (isPayment(transaction)) {
processPlanPayment();
} else if (isRefund(transaction)) {
processPlanRefund();
} else {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
}By doing this, we have eliminated the duplication of codes to a large extent. In the next step, we must define a guard to prevent the processing of invalid transactions. The right place to define guard is before the execution of the desired methods, or in other words inside the father method! With this account, we go to the processTransaction method and define our guard in it:
function processTransaction(transaction) {
if (!isOpen(transaction)) {
const error = new Error('Invalid transaction type.');
throw error;
}
if (!isPayment(transaction) && !isRefund(transaction)) {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
if (usesTransactionMethod(transaction, 'CREDIT_CARD')) {
processCreditCardTransaction(transaction);
} else if (usesTransactionMethod(transaction, 'PAYPAL')) {
processPayPalTransaction(transaction);
} else if (usesTransactionMethod(transaction, 'PLAN')) {
processPlanTransaction(transaction);
}
}In this method, I have defined a simple condition that says if the transaction is not payment or refund, then it is invalid and we throw a simple error inside it. How to make this error is also simple and has been explained several times so I won't repeat it again.
Now that we have thrown this error, where should we receive and handle it? If your answer is the main method, you are wrong! Why? Because this error is related to a specific transaction and should not stop the execution of all transactions. If we want to handle the error in main, the invalid transaction and all subsequent transactions will not be processed. With this account, the best place to handle this error is inside the for loop in the processTransactions method:
function processTransactions(transactions) {
if (isEmpty(transactions)) {
const error = new Error('No transactions provided!');
error.code = 1;
throw error;
}
for (const transaction of transactions) {
try {
processTransaction(transaction);
} catch (error) {
showErrorMessage(error.message, error.item);
}
}
}As you can see, by writing the error management code inside this for loop, we will reject the invalid transaction, but we will not harm other transactions. With this account, our codes will be as follows:
main();
function main() {
const transactions = [
{
id: 't1',
type: 'PAYMENT',
status: 'OPEN',
method: 'CREDIT_CARD',
amount: '23.99',
},
{
id: 't2',
type: 'PAYMENT',
status: 'OPEN',
method: 'PAYPAL',
amount: '100.43',
},
{
id: 't3',
type: 'REFUND',
status: 'OPEN',
method: 'CREDIT_CARD',
amount: '10.99',
},
{
id: 't4',
type: 'PAYMENT',
status: 'CLOSED',
method: 'PLAN',
amount: '15.99',
},
];
try {
processTransactions(transactions);
} catch (error) {
showErrorMessage(error.message);
}
}
function processTransactions(transactions) {
if (isEmpty(transactions)) {
const error = new Error('No transactions provided!');
error.code = 1;
throw error;
}
for (const transaction of transactions) {
try {
processTransaction(transaction);
} catch (error) {
showErrorMessage(error.message, error.item);
}
}
}
function isEmpty(transactions) {
return !transactions || transactions.length === 0;
}
function showErrorMessage(message, item) {
console.log(message);
if (item) {
console.log(item);
}
}
function processTransaction(transaction) {
if (!isOpen(transaction)) {
const error = new Error('Invalid transaction type.');
throw error
}
if (!isPayment(transaction) && !isRefund(transaction)) {
const error = new Error('Invalid transaction type!');
error.item = transaction;
throw error;
}
if (usesTransactionMethod(transaction, 'CREDIT_CARD')) {
processCreditCardTransaction(transaction);
} else if (usesTransactionMethod(transaction, 'PAYPAL')) {
processPayPalTransaction(transaction);
} else if (usesTransactionMethod(transaction, 'PLAN')) {
processPlanTransaction(transaction);
}
}
function isOpen(transaction) {
return transaction.status === 'OPEN';
}
function usesTransactionMethod(transaction, method) {
return transaction.method === method;
}
function isPayment(transaction) {
return transaction.type === 'PAYMENT';
}
function isRefund(transaction) {
return transaction.type === 'REFUND';
}
function processCreditCardTransaction(transaction) {
if (isPayment(transaction)) {
processCreditCardPayment();
} else if (isRefund(transaction)) {
processCreditCardRefund();
}
}
function processPayPalTransaction(transaction) {
if (isPayment(transaction)) {
processPayPalPayment();
} else if (isRefund(transaction)) {
processPayPalRefund();
}
}
function processPlanTransaction(transaction) {
if (isPayment(transaction)) {
processPlanPayment();
} else if (isRefund(transaction)) {
processPlanRefund();
}
}
function processCreditCardPayment(transaction) {
console.log(
'Processing credit card payment for amount: ' + transaction.amount
);
}
function processCreditCardRefund(transaction) {
console.log(
'Processing credit card refund for amount: ' + transaction.amount
);
}
function processPayPalPayment(transaction) {
console.log('Processing PayPal payment for amount: ' + transaction.amount);
}
function processPayPalRefund(transaction) {
console.log('Processing PayPal refund for amount: ' + transaction.amount);
}
function processPlanPayment(transaction) {
console.log('Processing plan payment for amount: ' + transaction.amount);
}
function processPlanRefund(transaction) {
console.log('Processing plan refund for amount: ' + transaction.amount);
}