Recently, Xiao Li felt that the girls at his office were looking at him in the wrong way, and the smile seemed to be a loving, aunt-like one.
As an honest programmer, Li wasn’t used to the attention and didn’t know what was going on.
· · · · · ·
Li and Wang seem to be too close to each other and often work together in the same cubicle until midnight.
This myth until one day their chat content was heard by a small sister, the secret between them was discovered by everyone.
The story of Xiao Li and Xiao Wang
“This code doesn’t look very good.” Xiao Li points at the screen, frowning.
“Why, I wrote this code because it looks a little complicated to implement a custom requirement from the customer.” Xiao Wang leaned over.
“I can’t put my finger on what’s wrong. I’m going to add a new feature and I don’t know where to start.” Xiao Li scratched his head.
Xiao Wang moved the stool over: “come, I tell you, this code, this call…..”
…
At 12 o ‘clock in the afternoon, the office air is filled with the aroma of food, there are groups of friends about the meal, leisure area has become the main battlefield of dry rice.
“Oh? Xiao Li and Xiao Wang this is called pair programming?” Operation little sister eyes sweep to still on the seat of xiao Li Xiao Wang.
“Well… Occasional pair programming is normal, but long-term pair programming shows some bad taste.” The old engineer said, picked up the bowl and finished the last mouthful of rice.
“Oh… Is that… Does it taste?” Operation little elder sister temptation way.
The HR and UI sisters who had dinner together were ignited by this sentence and gave out a burst of happy laughter.
“NoNoNo… What Geng means is that there is a bad smell in the code.” Senior engineer Dabao adjusted his glasses.
“What do you mean by a bad smell in your code?”
“It’s a sign that software architecture is starting to fall apart, that code is full of bad smells, and those smells are disgusting, rotten signals that you need to refactor.”
The young ladies frowned and were not satisfied with the appetizing example dabao told them during the meal.
Geng took a sip of water and put his glass down. “Po’s right. Apparently, Li and Wang didn’t see the bad smell in the code when they were writing it, and now they have a huge technical debt that they are paying off in instalments.”
“It seems like it’s time to train them on how to recognize bad smells in code.”
Bad smell in the code
Xiao Li and Xiao Wang came to the conference room with two big black eyes, old Geng had been waiting in the conference room.
Li took a look at the projector’s contents: “24 Common Bad Smells and Reconstruction Techniques: Goodbye to Overtime, Sustainable Development”.
“Now, coffee for you two, cheer up.” Dabao pushed the door open with his body and came in with two cups of steaming coffee in his hand, which he had just made by himself from the rest area.
“Thank you, Brother Bao,” said Xiao Li and Xiao Wang together.
“I hear you two have been working late together lately.” Mr. Geng took his hands off the keyboard and looked away from the computer screen at Mr. Li.
“Hey…” Xiao Li embarrassed smile “recently the demand is more difficult, add some new function more spend time.”
“Well, it looks like you guys are seeing some problems. Why is it taking so much more time to add features now than it used to?” Geng pushed back his stool and looked at them.
“Because products get more complex, it takes more time to add new features.” “Xiao Wang answered.
“Yes, that’s one reason. What else?” “Geng asked.
“Because… Because the code we wrote was a little messy, and it might take time to understand each other…” Xiao Li scratched his head.
“But I annotated them, I think, because the product was so complicated.” Xiao Wang is somewhat disapproving of Xiao Li’s statement.
“Good, actually xiao Li said a key point.” Geng decided it was time, and continued: “Martin Fowler once said that when a code base looks like a stack of patches, it takes careful archaeology to figure out how the whole system works. That burden slows down the pace of new features until programmers want to rewrite the entire system from scratch.”
“I suppose there are many times when you have felt the urge to rewrite the system?” Geng smiled and continued, “And with good internal quality software, when I add new features, it’s easy to find out where and how to modify them.”
Old Geng paused, “So, what Xiao Wang said is also right. Product complexity brings with it software complexity, and complex software is easy to become patch pile patch accidentally. Software code is filled with ‘bad smells’ that eventually rot the software and lose control of it.”
“By the way, xiao Wang just mentioned the annotation, is also a bad taste.” Geng smiled and looked at Wang. “So, bad tastes are everywhere. Some bad tastes are easier to detect, and some bad tastes require special training.”
Geng again pointed to the large projector screen. “So, today we have a special training course that will teach you to identify 24 common bad smells and refactoring techniques. This course will at least make you a good programmer with some particularly good habits.”
“Ahem, you two have made a lot of money,” Dabao added. “Geng’s level, you have to charge for classes outside.”
“Wait, I’ll get my notebook.” Xiao Li raised his hand, and then opened the door and went out. Xiao Wang followed Xiao Li out to take a notebook.
24 common bad taste and refactoring techniques
When Lao Geng saw that everyone was ready to learn, he stood up and walked to the screen and said to everyone: “Let’s start!”
“Bad taste we’ve just talked about, let me tell you a little story. Over the years, I’ve seen many, many pieces of code from projects that have been both successful and moribund. When we look at this code, my old partner and I learn to pick out certain structures that indicate the possibility of refactoring, and those structures are the bad smell I just mentioned.”
“Oh yeah, I also mentioned the word refactoring. This sounds like a terrible word, but I can say with you, my mouth refactoring is not the toppling over again, you want to have a book to a new definition of the word – the internal structure of a kind of adjustment to the software, the purpose is without changing the software under the premise of observable behavior, improve the understandability, decrease the cost of its modification. “You could also think of it as using a series of refactoring techniques to adjust the structure of the software without changing its observable behavior.”
“If someone says their code is not available for a day or two during refactoring, it’s pretty much a certainty that what they’re doing is not refactoring.” Geng joked: “They may be working some kind of healing magic on the code, and the side effect of that magic is to send the software into temporary shock.”
“Another thing worth mentioning here is how to do this without changing the observable behavior of the software, which requires some assistance. I’m not suggesting that you extend overtime to testers, but rather that you have a complete, fast test suite. In most cases, if you want to refactor, you need a set of code that can test itself.”
“Next, I will first review the code of your mall system, find the existence of bad taste in the code, then add unit tests, refactor, and finally complete the refactor through the test.”
“In the process, I’m going to show you and explain 24 common bad smells and refactoring techniques.”
“Let’s get started! Old Geng returned to his seat.
Mysterious names
function countOrder(order) {
const basePrice = order.quantity * order.itemPrice;
const quantityDiscount = Math.max(0, order.quantity - 500) * order.itemPrice * 0.05;
const shipping = Math.min(basePrice * 0.1.100);
return basePrice - quantityDiscount + shipping;
}
const orderPrice = countOrder(order);
Copy the code“What does this countOrder function do? The first impression of the function name is not clear, statistical order? The quantity of the order? Or statistics? However, after looking at the internal implementation of the function, I realize that this is a function that counts the total price of the order. That’s one of the bad smells — cryptic naming, and when you have more of that in code, you’re going to spend more and more time guessing.”
“To refactor this code, we need to add the unit test code first. I’ll just do a demo here and write two test cases.”
Geng quickly wrote the following two test cases for this code using the well-known JEST framework.
describe('test price'.() = > {
test('countOrder should return normal price when input correct order quantity < 500'.() = > {
const input = {
quantity: 20.itemPrice: 10
};
const result = countOrder(input);
expect(result).toBe(220);
});
test('countOrder should return discount price when input correct order quantity > 500'.() = > {
const input = {
quantity: 1000.itemPrice: 10
};
const result = countOrder(input);
expect(result).toBe(9850);
});
});
Copy the codeAfter running the test cases and showing that they passed, Geng said, “Once we have the unit tests, we can start preparing for the refactoring.”
“Let’s start by refining the internal countOrder implementation into a new function called getPrice, which is not necessarily the most appropriate name, but it will be better than the previous one.” Geng accomplished this step easily with his Ide.
function getPrice(order) {
const basePrice = order.quantity * order.itemPrice;
const quantityDiscount = Math.max(0, order.quantity - 500) * order.itemPrice * 0.05;
const shipping = Math.min(basePrice * 0.1.100);
return basePrice - quantityDiscount + shipping;
}
function countOrder(order) {
return getPrice(order);
}
const orderPrice = countOrder(order);
Copy the code“This step looks fine, but let’s run the test case first.” Lao Geng pressed the shortcut key to execute the use case, the use case ran, and soon passed the test.
“This step shows that our change is ok. Next, we modify the test case, change the countOrder method called by the test case to call the getPrice method, and run the modified test case again.”
Geng pointed to the modified test case: “After running it again, getPrice also passed the test, so we can change the call to getPrice wherever we call countOrder, like this.”
function getPrice(order) {
const basePrice = order.quantity * order.itemPrice;
const quantityDiscount = Math.max(0, order.quantity - 500) * order.itemPrice * 0.05;
const shipping = Math.min(basePrice * 0.1.100);
return basePrice - quantityDiscount + shipping;
}
function countOrder(order) {
return getPrice(order);
}
const orderPrice = getPrice(order);
Copy the code“At this point we can see that the editor has informed us that the original countOrder method is not being used, and we can use the Ide to delete it directly.”
“Once removed, our refactoring is complete, and the new code looks like this.”
function getPrice(order) {
const basePrice = order.quantity * order.itemPrice;
const quantityDiscount = Math.max(0, order.quantity - 500) * order.itemPrice * 0.05;
const shipping = Math.min(basePrice * 0.1.100);
return basePrice - quantityDiscount + shipping;
}
const orderPrice = getPrice(order);
Copy the code“Well, that seems like a better name, because at first glance it’s a function that gets the price of an order. If you come up with a better name, replace it with the same steps, and have unit tests to ensure that you don’t make mistakes and cause problems along the way.”
“Yes, and the most important step.” Old Geng accentuates the tone: “remember to submit code!”
Geng, who uses the shortcut key to Commit a Commit record, continues: “It’s useful to Commit the code every time a refactoring is complete so that the next time something goes wrong with the refactoring, you can quickly fall back to where it was the last time it worked properly! “
Dabao added: “The benefit of this refactoring is that the code is always releasable, as there is no impact on overall functionality.”
“But it’s not easy to come up with a good name,” geng said. “Let’s continue to refactor in small steps.” Xiao Wang touched his chin to think.
“Po and Wang are both right about continuing to refactor in small steps to keep the software ready for release without changing the observable behavior of the software. This means we can refactor at any time. The simplest refactorings, like the one I just demonstrated, should take less than a few minutes, while the longest should take less than a few hours.”
“Let me add one more thing.” “We can’t afford to ignore automated testing,” he says. “It’s the small, but most important, part of refactoring that doesn’t change the observable behavior of the software.”
“Po is right, we need to at least make sure that there are unit tests where we refactor, and that we pass unit tests, to count as refactoring done.”
Geng paused for a moment, waiting for everyone to understand what he had just said, and then said: “It seems that everyone began to feel the charm of refactoring. Let’s finally look at the comparison of the code before and after refactoring.”
“Ok, let’s move on to the rest of the bad smell.”
Repeat Code
function renderPerson(person) {
const result = [];
result.push(`<p>${person.name}</p>`);
result.push(`<p>title: ${person.photo.title}</p>`);
result.push(emitPhotoData(person.photo));
return result.join('\n');
}
function photoDiv(photo) {
return ['<div>'.`<p>title: ${photo.title}</p>`, emitPhotoData(photo), '</div>'].join('\n');
}
function emitPhotoData(aPhoto) {
const result = [];
result.push(`<p>location: ${aPhoto.location}</p>`);
result.push(`<p>date: ${aPhoto.date}</p>`);
return result.join('\n');
}
Copy the code“Well, at first glance there’s nothing wrong with this generation, and it’s supposed to be used to render the profile interface. But if we look closely, we’ll see that the renderPerson method has the same implementation as the photoDiv method, which is the part that renders photo.title. This part of the logic always precedes the execution of the emitPhotoData function, which is a piece of repetitive code.”
“This is a seemingly innocuous piece of repetitive code, but remember that when repetitive code exists, you have to be very careful to read it and pay attention to the subtle differences. If you want to change duplicate code, you have to find all the copy changes, which makes it easy for someone to read and change the code and make mistakes.”
“So, we picked this piece of code to refactor. By convention, I write two unit test cases first.” Lao Geng started writing use cases.
describe('test render'.() = > {
test('renderPerson should return correct struct when input correct struct'.() = > {
const input = {
name: 'jack'.photo: {
title: 'travel'.location: 'tokyo'.date: '2021-06-08'}};const result = renderPerson(input);
expect(result).toBe(`<p>jack</p>\n<p>title: travel</p>\n<p>location: tokyo</p>\n<p>date: 2021-06-08</p>`);
});
test('photoDiv should return correct struct when input correct struct'.() = > {
const input = {
title: 'adventure'.location: 'india'.date: '2021-01-08'
};
const result = photoDiv(input);
expect(result).toBe(`<div>\n<p>title: adventure</p>\n<p>location: india</p>\n<p>date: 2021-01-08</p>\n</div>`);
});
});
Copy the code“Let’s run the test first to see if our test case passes. “Geng pressed the execution shortcut key.
“Ok, the test passed, make a Commit and save our test code. Now, we’re going to start refactoring, and this is a fairly simple function, so we can just move that repeating line of code directly into the emitPhotoData function. But this time we’ll show you a less risky refactoring technique to prevent mistakes. “Old Geng said, put emitPhotoDataNew CTRL C + CTRL V, in the copy function body slightly modified, completed the assembly.
function emitPhotoDataNew(aPhoto) {
const result = [];
result.push(`<p>title: ${aPhoto.title}</p>`);
result.push(`<p>location: ${aPhoto.location}</p>`);
result.push(`<p>date: ${aPhoto.date}</p>`);
return result.join('\n');
}
Copy the code“Then we replace the methods called internally by renderPerson and photoDiv with a new method called emitPhotoDataNew, and if it’s safe it’s better to do a test case with a new function.”
function renderPerson(person) {
const result = [];
result.push(`<p>${person.name}</p>`);
result.push(emitPhotoDataNew(person.photo));
return result.join('\n');
}
function photoDiv(photo) {
return ['<div>', emitPhotoDataNew(photo), '</div>'].join('\n');
}
function emitPhotoData(aPhoto) {
const result = [];
result.push(`<p>location: ${aPhoto.location}</p>`);
result.push(`<p>date: ${aPhoto.date}</p>`);
return result.join('\n');
}
function emitPhotoDataNew(aPhoto) {
const result = [];
result.push(`<p>title: ${aPhoto.title}</p>`);
result.push(`<p>location: ${aPhoto.location}</p>`);
result.push(`<p>date: ${aPhoto.date}</p>`);
return result.join('\n');
}
Copy the code“When the replacement is complete, execute the test case and see what happens.”
‘OK, the test passes and there is no problem with the refactoring, then delete the original emitPhotoData safely and rename emitPhotoDataNew to emitPhotoData and refactoring is done!’
function renderPerson(person) {
const result = [];
result.push(`<p>${person.name}</p>`);
result.push(emitPhotoData(person.photo));
return result.join('\n');
}
function photoDiv(photo) {
return ['<div>', emitPhotoData(photo), '</div>'].join('\n');
}
function emitPhotoData(aPhoto) {
const result = [];
result.push(`<p>title: ${aPhoto.title}</p>`);
result.push(`<p>location: ${aPhoto.location}</p>`);
result.push(`<p>date: ${aPhoto.date}</p>`);
return result.join('\n');
}
Copy the code“When you’re done, don’t forget to run the test cases.” Geng seems to have developed muscle memory by running test cases after each modification.
“Ok, the test passed. Now that the refactoring is complete, do a Commit, and see what happens before and after.”
“Let’s move on to the next bad taste.”
Long Function
function printOwing(invoice) {
let outstanding = 0;
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
console.log('**** Customer Owes ****');
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
// calculate outstanding
for (const o of invoice.orders) {
outstanding += o.amount;
}
// record due date
const today = new Date(Date.now());
invoice.dueDate = new Date(today.getFullYear(), today.getMonth(), today.getDate() + 30);
//print details
console.log(`name: ${invoice.customer}`);
console.log(`amount: ${outstanding}`);
console.log(`due: ${invoice.dueDate.toLocaleDateString()}`);
}
Copy the code“Well, this function looks like it’s used to print the user’s white stripes. The implementation details and naming of the function are fine, but there is a bad smell — long functions.”
“The longer the function, the harder it is to understand. Better explanatory power, easier to share, more choices — all supported by smaller functions.”
“There is a trick to identifying this bad smell. That is, if you need to spend time looking through a piece of code to figure out what it’s doing, you should extract it into a function and name it based on what it does.”
“A function like this is one of those things that takes a bit of browsing to figure out what it’s doing, but it’s nice to have some comments on it.”
“As usual, let’s write two test cases first.” Geng started typing code.
describe('test printOwing'.() = > {
let collections = [];
console.log = message= > {
collections.push(message);
};
afterEach(() = > {
collections = [];
});
test('printOwing should return correct struct when input correct struct'.() = > {
const input = {
customer: 'jack'.orders: [{ amount: 102 }, { amount: 82 }, { amount: 87 }, { amount: 128}}; printOwing(input); expect(collections).toStrictEqual(['* * * * * * * * * * * * * * * * * * * * * * *'.'**** Customer Owes ****'.'* * * * * * * * * * * * * * * * * * * * * * *'.'name: jack'.'amount: 399'.'due: 7/8/2021'
]);
});
test('printOwing should return correct struct when input correct struct 2'.() = > {
const input = {
customer: 'dove'.orders: [{ amount: 63 }, { amount: 234 }, { amount: 12 }, { amount: 1351}}; printOwing(input); expect(collections).toStrictEqual(['* * * * * * * * * * * * * * * * * * * * * * *'.'**** Customer Owes ****'.'* * * * * * * * * * * * * * * * * * * * * * *'.'name: dove'.'amount: 1660'.'due: 7/8/2021'
]);
});
});
Copy the code“Run the test case after you’ve written it. “
“The next step of extraction is easy, because the code itself is commented, and we just need to follow the rhythm of the annotation to extract it. It’s still a little jog, first adjusting the order of function execution, layering functions.”
function printOwing(invoice) {
// print banner
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
console.log('**** Customer Owes ****');
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
// calculate outstanding
let outstanding = 0;
for (const o of invoice.orders) {
outstanding += o.amount;
}
// record due date
const today = new Date(Date.now());
invoice.dueDate = new Date(today.getFullYear(), today.getMonth(), today.getDate() + 30);
// print details
console.log(`name: ${invoice.customer}`);
console.log(`amount: ${outstanding}`);
console.log(`due: ${invoice.dueDate.toLocaleDateString()}`);
}
Copy the code“After layering functions, run the test cases first to prevent function functions from being affected during the process of adjusting the order… Ok, the test passed.”
“The reordered function becomes very simple, so we can extract it in four steps.”
“First, refine the printBanner function and run the test case.”
function printBanner() {
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
console.log('**** Customer Owes ****');
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
}
function printOwing(invoice) {
printBanner();
// calculate outstanding
let outstanding = 0;
for (const o of invoice.orders) {
outstanding += o.amount;
}
// record due date
const today = new Date(Date.now());
invoice.dueDate = new Date(today.getFullYear(), today.getMonth(), today.getDate() + 30);
// print details
console.log(`name: ${invoice.customer}`);
console.log(`amount: ${outstanding}`);
console.log(`due: ${invoice.dueDate.toLocaleDateString()}`);
}
Copy the code“In the process of extraction, we removed the comment because it was no longer needed. The name of the function is the same as the content of the comment.”
“Step two, refine the calOutstanding function, then run the test case.”
function printBanner() {
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
console.log('**** Customer Owes ****');
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
}
function calOutstanding(invoice) {
let outstanding = 0;
for (const o of invoice.orders) {
outstanding += o.amount;
}
return outstanding;
}
function printOwing(invoice) {
printBanner();
let outstanding = calOutstanding(invoice);
// record due date
const today = new Date(Date.now());
invoice.dueDate = new Date(today.getFullYear(), today.getMonth(), today.getDate() + 30);
// print details
console.log(`name: ${invoice.customer}`);
console.log(`amount: ${outstanding}`);
console.log(`due: ${invoice.dueDate.toLocaleDateString()}`);
}
Copy the code“Step three, refine the recordDueDate function and run the test case.”
function printBanner() {
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
console.log('**** Customer Owes ****');
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
}
function calOutstanding(invoice) {
let outstanding = 0;
for (const o of invoice.orders) {
outstanding += o.amount;
}
return outstanding;
}
function recordDueDate(invoice) {
const today = new Date(Date.now());
invoice.dueDate = new Date(today.getFullYear(), today.getMonth(), today.getDate() + 30);
}
function printOwing(invoice) {
printBanner();
let outstanding = calOutstanding(invoice);
recordDueDate(invoice);
// print details
console.log(`name: ${invoice.customer}`);
console.log(`amount: ${outstanding}`);
console.log(`due: ${invoice.dueDate.toLocaleDateString()}`);
}
Copy the code“Step four, refine the printDetails function and run the test case.”
function printBanner() {
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
console.log('**** Customer Owes ****');
console.log('* * * * * * * * * * * * * * * * * * * * * * *');
}
function calOutstanding(invoice) {
let outstanding = 0;
for (const o of invoice.orders) {
outstanding += o.amount;
}
return outstanding;
}
function recordDueDate(invoice) {
const today = new Date(Date.now());
invoice.dueDate = new Date(today.getFullYear(), today.getMonth(), today.getDate() + 30);
}
function printDetails(invoice, outstanding) {
console.log(`name: ${invoice.customer}`);
console.log(`amount: ${outstanding}`);
console.log(`due: ${invoice.dueDate.toLocaleDateString()}`);
}
function printOwing(invoice) {
printBanner();
let outstanding = calOutstanding(invoice);
recordDueDate(invoice);
printDetails(invoice, outstanding);
}
Copy the code
“Don’t forget to commit the code after the test case passes.”
“Then let’s take a look at this refactoring printOwing function, simple four lines of code, clearly describes what the function does, this is the charm of small functions!”
“At the end of the day, the key to making small functions easy to understand is good naming. If you can give a function a good name, the person reading the code will know what the function does by the name without having to look at what’s in it. It saves a lot of time, and it saves you the time you have to pair program.” Geng smiled at Xiao Li and Xiao Wang.
Xiao Li and Xiao Wang smiled at each other, feeling a little embarrassed.
“Let’s look at before and after the refactoring.”
“Here we go.” Geng never stops.
Long Parameter List
// range.js
function priceRange(products, min, max, isOutSide) {
if (isOutSide) {
return products
.filter(r= > r.price < min || r.price > max);
} else {
return products
.filter(r= >r.price > min && r.price < max); }}// a.js
const range = { min: 1.max: 10 }
const outSidePriceProducts = priceRange(
[ / *... * / ],
range.min,
range.max,
true
)
// b.js
const range = { min: 5.max: 8 }
const insidePriceProducts = priceRange(
[ / *... * / ],
range.min,
range.max,
false
)
Copy the code“At first glance, priceRange is a function of filtering out items. On closer inspection, the main comparison is between the product.price field and the input parameters min and Max. If isOutSide is true, filter out items outside the price range, otherwise filter out items within the price range.”
“At first glance, this function has too many arguments, which can confuse the client caller. It’s a good thing the isOutSide parameter is named well, otherwise the function would look more esoteric.”
“I have to take a look at priceRange’s implementation every time I call it, and I keep forgetting the rule for the last argument.”
“My opinion is the same as Xiao Li’s.” Geng nodded. “I don’t like marking parameters either, because they make it difficult to understand what functions can be called and how. With a function like this, I also have to figure out what values are available for the marker argument.”
“Now that Xiao Li has found this problem, let’s start with the isOutSide parameter and optimize it. As usual, we write two test cases against existing code.” Geng started writing code.
describe('test priceRange'.() = > {
test('priceRange should return correct result when input correct outside conditional'.() = > {
const products = [
{ name: 'apple'.price: 6 },
{ name: 'banana'.price: 7 },
{ name: 'orange'.price: 15 },
{ name: 'cookie'.price: 0.5}];const range = { min: 1.max: 10 };
const isOutSide = true;
const result = priceRange(products, range.min, range.max, isOutSide);
expect(result).toStrictEqual([
{ name: 'orange'.price: 15 },
{ name: 'cookie'.price: 0.5}]); }); test('priceRange should return correct result when input correct inside conditional'.() = > {
const products = [
{ name: 'apple'.price: 6 },
{ name: 'banana'.price: 7 },
{ name: 'orange'.price: 15 },
{ name: 'cookie'.price: 0.5}];const range = { min: 5.max: 8 };
const isOutSide = false;
const result = priceRange(products, range.min, range.max, isOutSide);
expect(result).toStrictEqual([
{ name: 'apple'.price: 6 },
{ name: 'banana'.price: 7}]); }); });Copy the code“Run the unit tests… Yes, it is. Let’s start with the argument simplification. Let’s fix the problem li mentioned earlier, which is marking parameters, and refine two more functions for priceRange.”
“Let’s start by modifying our unit test code the way we expect it to be called.”
const priceRange = require('./long_parameter_list');
describe('test priceRange'.() = > {
test('priceOutSideRange should return correct result when input correct outside conditional'.() = > {
const products = [
{ name: 'apple'.price: 6 },
{ name: 'banana'.price: 7 },
{ name: 'orange'.price: 15 },
{ name: 'cookie'.price: 0.5}];const range = { min: 1.max: 10 };
const result = priceOutSideRange(products, range.min, range.max);
expect(result).toStrictEqual([
{ name: 'orange'.price: 15 },
{ name: 'cookie'.price: 0.5}]); }); test('priceInsideRange should return correct result when input correct inside conditional'.() = > {
const products = [
{ name: 'apple'.price: 6 },
{ name: 'banana'.price: 7 },
{ name: 'orange'.price: 15 },
{ name: 'cookie'.price: 0.5}];const range = { min: 5.max: 8 };
const result = priceInsideRange(products, range.min, range.max);
expect(result).toStrictEqual([
{ name: 'apple'.price: 6 },
{ name: 'banana'.price: 7}]); }); });Copy the code“I removed the isOutSide tag from priceRange and used the priceOutsideRange and priceInsideRange methods to do the same. We can’t run the test case at this point because our code hasn’t changed yet. Again, tweak the code to fit the use case call.”
function priceRange(products, min, max, isOutSide) {
if (isOutSide) {
return products.filter(r= > r.price < min || r.price > max);
} else {
return products.filter(r= >r.price > min && r.price < max); }}function priceOutSideRange(products, min, max) {
return priceRange(products, min, max, true);
}
function priceInsideRange(products, min, max) {
return priceRange(products, min, max, false);
}
Copy the code“With the code tweaked, let’s run the test case. All right, it’s passed!”
“Well, I think once we get there, we can take it a step further and take priceRange’s functions a step further, like this.”
function priceRange(products, min, max, isOutSide) {
if (isOutSide) {
return products.filter(r= > r.price < min || r.price > max);
} else {
return products.filter(r= >r.price > min && r.price < max); }}function priceOutSideRange(products, min, max) {
return products.filter(r= > r.price < min || r.price > max);
}
function priceInsideRange(products, min, max) {
return products.filter(r= > r.price > min && r.price < max);
}
Copy the code“When you’re done, remember to run the test cases… Ok, it’s passed.”
“After the test cases pass, you can begin to prepare for the migration. Replace the old call to priceRange with the new call, and then remove priceRange safely, like this.”
// range.js
function priceOutSideRange(products, min, max) {
return products.filter(r= > r.price < min || r.price > max);
}
function priceInsideRange(products, min, max) {
return products.filter(r= > r.price > min && r.price < max);
}
// a.js
const range = { min: 1.max: 10 }
const outSidePriceProducts = priceOutSideRange(
[ / *... * / ],
range.min,
range.max
)
// b.js
const range = { min: 5.max: 8 }
const insidePriceProducts = priceInsideRange(
[ / *... * / ],
range.min,
range.max
)
Copy the code“By doing so, the confusing tag arguments have been removed and replaced by two functions with clearer semantics.”
“Next, we continue to do a valuable refactoring, which is to organize the data into a structure, because it makes the relationships between the data items clear. For example, the min and Max of range are always used together in the call, so the two parameters can be organized into a structure. I modify my test cases to accommodate the latest change, like this.”
/ /...
const range = { min: 1.max: 10 };
const result = priceOutSideRange(products, range);
expect(result).toStrictEqual([
{ name: 'orange'.price: 15 },
{ name: 'cookie'.price: 0.5}]);/ /...
const range = { min: 5.max: 8 };
const result = priceInsideRange(products, range);
expect(result).toStrictEqual([
{ name: 'apple'.price: 6 },
{ name: 'banana'.price: 7}]);Copy the code“When the test case changes are complete, modify our function.”
// range.js
function priceOutSideRange(products, range) {
return products.filter(r= > r.price < range.min || r.price > range.max);
}
function priceInsideRange(products, range) {
return products.filter(r= > r.price > range.min && r.price < range.max);
}
// a.js
const range = { min: 1.max: 10 }
const outSidePriceProducts = priceOutSideRange(
[ / *... * / ],
range
)
// b.js
const range = { min: 5.max: 8 }
const insidePriceProducts = priceInsideRange(
[ / *... * / ],
range
)
Copy the code“When the changes are done, run our test cases, pass them, and don’t forget to commit the code.” Say that finish, old Geng dozen a Commit.
“This step of refactoring reduces one more parameter, and this is the most immediate value of this refactoring. The real significance of this refactoring is that it leads to deeper changes in the code. Once the new data structures are identified, I can reorganize the program’s behavior to use those structures. What does this mean in practical terms? I’ll use this case as an example.”
“We’ll find that the priceOutSideRange and priceInsideRange functions are clear enough, but the internal determination of the range takes some time to understand, and range, as a structure we just identified, can be refactoring. Like this.”
// range.js
class Range {
constructor(min, max) {
this._min = min;
this._max = max;
}
outside(num) {
return num < this._min || num > this._max;
}
inside(num) {
return num > this._min && num < this._max; }}function priceOutSideRange(products, range) {
return products.filter(r= > range.outside(r.price));
}
function priceInsideRange(products, range) {
return products.filter(r= > range.inside(r.price));
}
// a.js
const outSidePriceProducts = priceOutSideRange(
[ / *... * /].new Range(1.10))// b.js
const insidePriceProducts = priceInsideRange(
[ / *... * /].new Range(5.8))Copy the code“Modify the test case and pass in the Range object, then run the test case… Ok, yes. Submit the code after the test passes.”
“This also makes the priceOutSideRange and priceInsideRange functions clearer. At the same time, the range is organized into a new data structure that can be used anywhere in the computing interval.”
“Let’s look at before and after the refactoring.”
“Here we go.”
Global Data
// global.js
// ...
let userAuthInfo = {
platform: 'pc'.token: ' '
}
export {
userAuthInfo
};
// main.js
userAuthInfo.token = localStorage.token;
// request.js
const reply = await login();
userAuthInfo.token = reply.data.token;
// business.js
await request({ authInfo: userAuthInfo });
Copy the code“This global.js seems to be designed to provide global data, which is one of the most pungent bad smells.”
“This platform is used globally. Can I change it to something else? Will it cause problems?” Asks the old geng
Xiao Li hurriedly said: “This platform can not be changed, the back end depends on this field to select the way to identify the token, change will cause problems.”
“But now I can change platforms and tokens in any corner of the code base, and there’s no mechanism to detect which piece of code has changed. It’s a global data problem.”
“Whenever we see data that might be contaminated by code here and there, we still need to wrap the global data in a function. At least you can see where it’s being modified and start controlling access to it. I’ll do a simple encapsulation here and then write two test cases.”
let userAuthInfo = {
platform: 'pc'.token: ' '
};
function getUserAuthInfo() {
return { ...userAuthInfo };
}
function setToken(token) {
userAuthInfo.token = token;
}
export {
getUserAuthInfo,
setToken
}
// main.js
setToken(localStorage.token);
// request.js
const reply = await login();
setToken(reply.data.token);
// business.js
await request({ authInfo: getUserAuthInfo() });
Copy the code“Now run the test case.”
describe("test global data".() = > {
test('getUserAuthInfo.platform should return pc when modify reference'.() = > {
const userAuthInfo = getUserAuthInfo();
userAuthInfo.platform = 'app';
const result = getUserAuthInfo().platform;
expect(result).toBe('pc');
});
test('getUserInfo.token should return test-token when setToken test-token'.() = > {
setToken('test-token');
const result = getUserAuthInfo().token;
expect(result).toBe('test-token');
});
});
Copy the code
“As a result, the source object cannot be modified by object reference, and I control the modification of the platform property here, only the modification of the token interface is open. Even so, global data should be avoided as much as possible, because global data is one of the most pungent bad smells!” Old Geng’s tone accentuated.
Xiao Li xiao Wang nodded wildly.
“Let’s look at before and after refactoring.”
“Let’s go on, then.”
Mutable Data
function merge(target, source) {
for (const key in source) {
target[key] = source[key];
}
return target;
}
Copy the code“This function seems a little old.” Old Geng was confused.
“This is a utility function that I copied from the previous repository, used to synthesize objects, and it hasn’t changed.” “Wang added.
“Well, the problem with this function is that it makes changes to target, the source object of the merge object. Changes to the data often result in unexpected results and bugs that are hard to find. At this point, the program doesn’t appear to be faulty because of this function, but if the fault occurs only in rare cases, it becomes more difficult to find the cause.”
“Write two test cases to verify this.” Geng started writing code.
describe('test merge'.() = > {
test('test merge should return correct struct when merge'.() = > {
const baseConfig = {
url: 'https://api.com'.code: 'mall'
};
const testSpecialConfig = {
url: 'https://test-api.com'.code: 'test-mall'
};
const result = merge(baseConfig, testSpecialConfig);
expect(result).toStrictEqual({
url: 'https://test-api.com'.code: 'test-mall'
});
});
test('test merge should return original struct when merge'.() = > {
const baseConfig = {
url: 'https://api.com'.code: 'mall'
};
const testSpecialConfig = {
url: 'https://test-api.com'.code: 'test-mall'
};
merge(baseConfig, testSpecialConfig);
expect(baseConfig).toStrictEqual({
url: 'https://api.com'.code: 'mall'
});
});
});
Copy the code“Run it… The second use case reported an error.”
“The reason for the error is because the source object was modified to affect the baseConfig value. The next thing you need to do is adjust the merge function. Javascript now has an easy way to modify this function.”
function merge(target, source) {
return{... target, ... source } }Copy the code“When the change is complete, run the use case again and see that the use case runs through.”
“I just refactorings in fact there was an entire genre – functional programming software development, is fully established on the basis of the concept of” data it will never change “: if you want to update a data structure, it returns a copy of the new data, old data remain unchanged, so that you can avoid a lot of problems caused by data changing.”
“Encapsulating variables, as I’ve just shown you with global data, is a common solution to the bad smell of mutable data. Also, if the value of the variable data can be calculated elsewhere, this is a particularly pungent bad smell. Not only is it annoying, buggy, and overtime, but it’s unnecessary.”
“I won’t go into it here, but if you two are interested, you can check out Refactoring: Improving the Design of Existing Code, 2nd Edition, which has all the bad smells I just mentioned.”
Xiao Li Xiao Wang penned the book and took down the title.
“Let’s look at before and after refactoring.”
“Let’s go on, then.”
Divergent changes
function getPrice(order) {
const basePrice = order.quantity * order.itemPrice;
const quantityDiscount = Math.max(0, order.quantity - 500) * order.itemPrice * 0.05;
const shipping = Math.min(basePrice * 0.1.100);
return basePrice - quantityDiscount + shipping;
}
const orderPrice = getPrice(order);
Copy the code“This is one of the earliest functions we reconstructed, and its job is to calculate the base price – quantity discount + shipping rate. If we look at this function, we need to change it if the base price rules change, we need to change it if the discount rules change, and we need to change it if the freight rules change.”
“If a module is constantly changing in different directions for different reasons, divergent changes occur.”
“The test cases are already there, so we can refactor the function directly.” Geng started writing code.
function calBasePrice(order) {
return order.quantity * order.itemPrice;
}
function calDiscount(order) {
return Math.max(0, order.quantity - 500) * order.itemPrice * 0.05;
}
function calShipping(basePrice) {
return Math.min(basePrice * 0.1.100);
}
function getPrice(order) {
return calBasePrice(order) - calDiscount(order) + calShipping(calBasePrice(order));
}
const orderPrice = getPrice(order);
Copy the code“When the changes are done, we run the test cases we wrote earlier… The test passed.”
“The three functions are only concerned with the direction of their responsibilities, rather than one function concerned with multiple directions. In addition, the unit test granularity can also be written a little more detailed, so the efficiency of troubleshooting is also greatly improved.”
Dabao timely added: “In fact, this is the single responsibility principle of object-oriented design principles.”
“Po’s right. Keep simple, it’s always important to focus on one context at a time.”
“Let’s look at before and after refactoring.”
“Here we go.”
Shotgun Surgery
// File Reading.js
const reading = {customer: "ivan".quantity: 10.month: 5.year: 2017};
function acquireReading() { return reading };
function baseRate(month, year) {
/ * * /
}
// File 1
const aReading = acquireReading();
const baseCharge = baseRate(aReading.month, aReading.year) * aReading.quantity;
// File 2
const aReading = acquireReading();
const base = (baseRate(aReading.month, aReading.year) * aReading.quantity);
const taxableCharge = Math.max(0, base - taxThreshold(aReading.year));
function taxThreshold(year) { / * * / }
// File 3
const aReading = acquireReading();
const basicChargeAmount = calculateBaseCharge(aReading);
function calculateBaseCharge(aReading) {
return baseRate(aReading.month, aReading.year) * aReading.quantity;
}
Copy the code“The next example of divergent change is the grape-shot change. It would take some time to find a software example, but I’m going to use an example from the original book Refactoring.”
“The problem is similar to repeated code, which often leads to a shotgun fix problem.”
“As shown in the above demo code, if part of the logic in reading is changed, the modification to that part of the logic needs to span several file adjustments.”
“If you have to make a lot of small changes in a lot of different classes or files for every change, you’re going to get the bad smell of shotgun change. If the code that needs to be changed is scattered around, it’s not only harder to find it, it’s easier to miss an important change.”
“Here I come to this code for refactoring, because the source code is not very complete, here I only put the idea of modification mentioned, do not write test code.” Geng started writing code.
// File Reading.js
class Reading {
constructor(data) {
this._customer = data.customer;
this._quantity = data.quantity;
this._month = data.month;
this._year = data.year;
}
get customer() {
return this._customer;
}
get quantity() {
return this._quantity;
}
get month() {
return this._month;
}
get year() {
return this._year;
}
get baseRate() {
/ *... * /
}
get baseCharge() {
return baseRate(this.month, this.year) * this.quantity;
}
get taxableCharge() {
return Math.max(0, base - taxThreshold());
}
get taxThreshold() {
/ *... * /}}const reading = new Reading({ customer: 'ivan'.quantity: 10.month: 5.year: 2017 });
Copy the code“After the changes are done, all the logic associated with reading is managed together, and I have the added benefit of grouping it into a single class. The class explicitly provides a common environment for these functions, simplifying function calls by passing fewer arguments inside the object, and making such an object more easily passed to the rest of the system.”
“If you have any of the problems I’ve just mentioned in the coding process, it’s a bad taste. Next time you can use a similar refactoring technique, don’t forget to write test cases.” Old Geng said to xiao Li two people.
Xiao Li xiao Wang nodded wildly.
“Let’s look at before and after refactoring.”
“Let’s go on, then.”
Feature Envy
class Account {
constructor(data) {
this._name = data.name;
this._type = data.type;
}
get loanAmount() {
if (this._type.type === 'vip') {
return 20000;
} else {
return 10000; }}}class AccountType {
constructor(type) {
this._type = type;
}
get type() {
return this._type; }}Copy the code“This code is the Account Account and the Account type AccountType. If the Account type is VIP, the loanAmount is 20,000, otherwise it is only 10,000.”
“The loanAmount method within the Account interacts with the internal data of another AccountType class more frequently than it does within the module in which it is located when it comes to obtaining the loanAmount, which is a typical case of attachment.”
“Let’s write two test cases first.” Geng started writing code.
describe('test Account'.() = > {
test('Account should return 20000 when input vip type'.() = > {
const input = {
name: 'jack'.type: new AccountType('vip')};const result = new Account(input).loanAmount;
expect(result).toBe(20000);
});
test('Account should return 20000 when input normal type'.() = > {
const input = {
name: 'dove'.type: new AccountType('normal')};const result = new Account(input).loanAmount;
expect(result).toBe(10000);
});
});
Copy the code“Test cases can run directly… Ok, passed.”
“Next, let’s move loanAmount to where it really belongs.”
class Account {
constructor(data) {
this._name = data.name;
this._type = data.type;
}
get loanAmount() {
return this._type.loanAmount; }}class AccountType {
constructor(type) {
this._type = type;
}
get type() {
return this._type;
}
get loanAmount() {
if (this.type === 'vip') {
return 20000;
} else {
return 10000; }}}Copy the code“After the move, loanAmount accesses data from its own module and no longer attaches to other modules. Let’s run the test case.”
“Ok, the test passed, don’t forget to commit the code.” Geng made a commit.
“Let’s look at before and after refactoring.”
“Let’s move on to the next one.”
1. Data Clumps
class Person {
constructor(name) {
this._name = name;
}
get name() {
return this._name;
}
set name(arg) {
this._name = arg;
}
get telephoneNumber() {
return ` (The ${this.officeAreaCode}) The ${this.officeNumber}`;
}
get officeAreaCode() {
return this._officeAreaCode;
}
set officeAreaCode(arg) {
this._officeAreaCode = arg;
}
get officeNumber() {
return this._officeNumber;
}
set officeNumber(arg) {
this._officeNumber = arg; }}const person = new Person('jack');
person.officeAreaCode = '+ 86';
person.officeNumber = 18726182811;
console.log(`person's name is ${person.name}, telephoneNumber is ${person.telephoneNumber}`);
// person's name is jack, telephoneNumber is (+86) 18726182811
Copy the code“The Person class records the user’s name, telephone area code, and phone number, which has a not-so-pungent bad taste.”
“If I remove the officeNumber field, then officeAreaCode is meaningless. This shows that the two fields always appear together, and that they are used in combination everywhere except in the Person class.”
“The bad taste is called data muddle, and it’s mainly the fact that data items tend to stay together in groups. You often see the same three or four pieces of data in many places: the same fields in two classes, the same arguments in many function signatures, and the data that comes together all the time really should have its own object.”
“As usual, we write two test cases first.” Geng started writing code
describe('test Person'.() = > {
test('person.telephoneNumber should return (+86) 18726182811 when input correct struct'.() = > {
const person = new Person('jack');
person.officeAreaCode = '+ 86';
person.officeNumber = 18726182811;
const result = person.telephoneNumber;
expect(person.officeAreaCode).toBe('+ 86');
expect(person.officeNumber).toBe(18726182811);
expect(result).toBe('(+ 86) 18726182811);
});
test('person.telephoneNumber should return (+51) 15471727172 when input correct struct'.() = > {
const person = new Person('jack');
person.officeAreaCode = '+ 51';
person.officeNumber = 15471727172;
const result = person.telephoneNumber;
expect(person.officeAreaCode).toBe('+ 51');
expect(person.officeNumber).toBe(15471727172);
expect(result).toBe('(51) + 15471727172');
});
});
Copy the code“Run the test case… Ok, test passed, ready to start refactoring.”
“Let’s create a new TelephoneNumber class to break up the responsibilities of the Person class.”
class TelephoneNumber {
constructor(areaCode, number) {
this._areaCode = areaCode;
this._number = number;
}
get areaCode() {
return this._areaCode;
}
get number() {
return this._number;
}
toString() {
return ` (The ${this._areaCode}) The ${this._number}`; }}Copy the code“At this point, let’s tweak our Person class to use the new data structure.”
class Person {
constructor(name) {
this._name = name;
this._telephoneNumber = new TelephoneNumber();
}
get name() {
return this._name;
}
set name(arg) {
this._name = arg;
}
get telephoneNumber() {
return this._telephoneNumber.toString();
}
get officeAreaCode() {
return this._telephoneNumber.areaCode;
}
set officeAreaCode(arg) {
this._telephoneNumber = new TelephoneNumber(arg, this.officeNumber);
}
get officeNumber() {
return this._telephoneNumber.number;
}
set officeNumber(arg) {
this._telephoneNumber = new TelephoneNumber(this.officeAreaCode, arg); }}Copy the code“With the refactoring done, we run the test code.”
“The test case passed. Don’t forget to commit the code.”
“I chose to create a new class here, rather than a simple record structure, because once you have a new class, you have a chance to make your program smell good. Once you have the new class, you can start looking for other bad flavors, such as “attachment plots,” which can help you point out behaviors that can be moved into the new class. It’s a powerful motivators: useful classes are created, a lot of duplication is eliminated, subsequent development is accelerated, and the old data muddle finally gets its full value in its small society.”
“Here, for example, the TelephoneNumber class is extracted so that it can eliminate duplicate code that uses TelephoneNumber and further optimize it based on usage. I won’t expand it.”
“Let’s look at before and after refactoring.”
“Let’s move on to the next bad taste.”
summary
Due to the word limit of the nugget, this article has to be published as “next and last”.
24 Common Bad Smells and Refactorings in Code (Part 2)
Or head over to Github to see 24 common bad smells and refactorings in code (full article)
One last thing
If you have already seen this, I hope you still click “like” before you go ~
Your “like” is the greatest encouragement to the author, and can also let more people see this article!
If you found this article helpful, please help to light up the Star on Github to encourage!