24 common bad smells and refactoring techniques in code

Recently, Xiao Li felt that the eyes of the company girls were not quite right when they saw him. The smile seemed to be a kind, aunt-like smile.

As an honest programmer, Xiao Li is not accustomed to this feeling of being over-concerned. He doesn't know what happened.

······

The relationship between Xiao Li and Xiao Wang seemed to be too close, and they often squeezed into a desk until midnight.

This rumor was not discovered until one day when the content of their chat was heard by an operating lady.

The story of Xiao Li and Xiao Wang

"This code doesn't look great." Xiao Li pointed at the screen, frowning.

"What's wrong, this code was written by me. It looks a bit complicated because it wants to implement a customized requirement of the customer." Xiao Wang leaned in.

"I can't tell what's wrong. I'm going to add a new feature now. I don't know where to start." Xiao Li scratched his head.

Xiao Wang moved the stool over: "Come on, let me tell you about this code, this call..."

·····

At 12 o'clock at noon, the air in the office was filled with the aroma of food, and there were groups of friends making appointments, and the leisure area became the main battlefield for dry meals.

"Oh? So Xiao Li and Xiao Wang are called pair programming?" The operating lady glanced at Xiao Li Xiao Wang who was still in her seat.

"Well...Occasionally pair programming is normal, but long-term pair programming shows that some bad smells have appeared." The old engineer finished speaking, took the rice bowl, and finished the last bite of the meal.

"Oh...is that...taste?" The operating lady tentatively asked.

The HR and UI ladies who were eating together were ignited by these words, and they made a burst of happy laughter.

"NoNoNo...Old Geng's meaning, there is a bad smell in the code." Senior engineer Dabao helped his glasses.

"What is the bad smell in the code?"

"It's a precursor to the beginning of the software architecture's disintegration. The code is full of bad smells. These smells are all disgusting and rotten signals, reminding you that it is time to refactor."

The little sisters frowned, not satisfied that Dabao told such an unappetizing example during the meal.

Old Geng took a sip of water and put down the water cup: "Abao is right. Obviously, when Xiao Li and Xiao Wang were writing the program, they did not notice the bad smell in the code, which caused them to owe a lot of money. The technical debt is being repaid in installments."

"It seems it's time to give them a training session and teach them how to recognize bad smells in the code."

Bad smell in code

Xiao Li and Xiao Wang came to the meeting room with two big black eyes. Old Geng had already been waiting in the meeting room.

Xiao Li glanced at the content of the projector " 24 common bad smells and reconstruction techniques-bid farewell to overtime, sustainable development".

"Now, the coffee prepared for you two, cheer up." Dabao pushed the door in with his body, holding two cups of hot coffee in his hand. This is the fresh coffee he has just brewed from the rest area.

"Thank you Brother Bao" Xiao Li and Xiao Wang said in unison.

"I heard that you two always work overtime together until midnight." Lao Geng took his hand away from the keyboard and moved his gaze from the computer screen to Xiao Li Xiao Wang's body.

"Hey..." Xiao Li smiled embarrassedly, "Recently the demand is more difficult, adding some new features takes more time."

"Well, it seems that you have also found some problems. Why does it take more time to add new features now than before?" Old Geng moved the stool back and looked at the two of them.

"Because products are becoming more and more complex, it takes more and more time to add new features." Xiao Wang answered.

"Yes, this is a reason. Is there any more?" Old Geng continued.

"Because the code we wrote before is a bit messy, it may take time to understand each other..." Xiao Li scratched his head.

"But I actually wrote comments. I think it's because the product is too complicated." Xiao Wang disagrees with Xiao Li's statement.

"Okay, actually Xiao Li mentioned a key issue." Old Geng thought it was time, and then said: "Martin Fowler once said that when the code base looks like a patch stack, it takes meticulous archaeological work to understand the whole How the system works. This burden will continue to slow down the speed of new features, and in the end programmers can't wait to rewrite the entire system from scratch."

"I think you should have the urge to rewrite the system many times, right?" Old Geng smiled and continued: "And good internal quality software allows me to easily find out where to modify and modify new features when adding new features. How to modify."

Old Geng paused, "So, Xiao Wang is right. The complexity of the product brings the complexity of the software, and complex software can easily become a patch stack if you are not careful. The software code is full of "bad smells." , These "bad smells" will eventually make the software rot, and then lose control of it."

"By the way, the comment that Xiao Wang just mentioned is also a bad smell." Old Geng smiled and looked at Xiao Wang, "So, bad smells are everywhere, some bad smells are easier to detect, and some are bad. The taste needs special training before it can be recognized."

Lao Geng pointed to the big screen of the projector again: "So, today we are a special training class to teach you to identify 24 common bad smells and reconstruction techniques. This class can at least make you a person with some particularly good A good programmer with a good habit."

"Ahem, you two have made a lot of money" Dabao added: "For a big cow of the level of old Geng, you have to charge for classes outside~"

"Wait, I'll get the notebook." Xiao Li raised his hand, then opened the door and walked out. Upon seeing this, Xiao Wang followed Xiao Li out to get the notebook.

24 common bad smells and reconstruction techniques

Seeing that everyone was ready to study, Old Geng stood up and walked to the side of the screen and said to everyone, "Then let's start!"

"We have already said the bad smell. Let me tell a little story. I have seen many codes over the years. The projects they belong to have been successful and some are dying. When observing these codes, I and My old partner learned to find certain specific structures from it. These structures pointed out the refactoring 160c833789dd09. These structures are the bad taste I just mentioned."

"Oh yes, I just mentioned a word- reconstruction. This sounds like a terrible word, but I can tell you that the reconstruction in my mouth is not the kind of overthrow you think. , There is a book that gives a brand-new definition of the term- is an adjustment to the internal structure of the software, the purpose is to improve the comprehensibility and reduce the cost of modification without changing the observable behavior of the software., You It can also be understood as adjust the structure of the software without changing the observable behavior of the software."

"If someone says that their code is unavailable for one or two days during the refactoring process, they can basically be sure that what they are doing is not refactoring." Old Geng made a joke: "They may be applying some kind of code to the code. Healing magic, the side effect of this magic is to make the software temporarily shocked."

"Here, there is another point worth mentioning, that is, how to do without changing the observable behavior of the software. This requires some external assistance. I do not recommend that you extend the overtime crowd to testers. I give The plan is to prepare a complete and fast-running test suite. In most cases, if you want to refactor, you must first have a set of self-testable code."

"Next, I will first review the code of your mall system and find out the bad smell in the code, then add unit tests, refactor, and finally complete the refactoring through testing."

"I will demonstrate and explain to you 24 common bad smells and reconstruction techniques during this process."

"Let's start!" Old Geng returned to his seat.

Mysterious Name

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);

" countOrder function name is not clear. It is not clear to count orders? Is the number of orders? Or statistics? However, after seeing the internal implementation of the function, I understand that this is a A function to count the total price of an order. This is one of the bad smells-the mysterious name . When there are more such bad smells in the code, more and more time will be spent on guessing."

"Let's refactor this code now. We need to add the unit test code first. Here I will only demonstrate and write two test cases."

Lao Geng quickly wrote the following two test cases for this code using the famous jest

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);
  });
});

Old Geng ran the test case, and after showing that the test passed, he said: "After we have the unit test, we can start preparing for refactoring."

"We first countOrder internal implementation of 060c833789df13 into a new function, named getPrice . This name is not necessarily the most suitable name, but it will be better than the previous one." Old Geng used Ide to easily complete this step.

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);

"This step does not seem to be a problem, but let's run the test case at first." Old Geng pressed the execute case shortcut, the case ran, and the test passed the test soon.

"This step shows that there is no problem with our modification. In the next step, we modify the test case, modify the countOrder method called by the test case to call the getPrice method, and run the modified test case ."

Geng pointed to the modified test case: "After the run again, getPrice also passed the test, then the next, we will be able to call countOrder method places, are modified to call getPrice method, 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);

"At this time, we can see that the editor has prompted us that the original countOrder method has not been used, and we can directly delete this function with the help of Ide."

"After deleting, 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);

"Well, this naming seems more appropriate. At a glance, you can tell that this is a function to get the order price. If a better name comes to mind someday, just replace it with the same steps. There is a unit. Testing can ensure that you will not make mistakes during the operation and cause other problems."

"By the way, there is the most important step." Old Gen increased his tone: "Remember to submit the code!"

Lao Geng submitted a Commit record with the shortcut key, and continued: "Every time the refactoring is completed, the code should be submitted, so that when there is a problem with the next refactoring, it can quickly fall back to the state of the previous normal work. It's very useful!"

Dabao added: "This refactoring has another advantage, that is, it can ensure that the code is in a release state at any time, because it does not affect the operation of the overall function."

"But it's really not easy to think of a suitable and good name. Brother Geng meant that we should continue to refactor in small steps." Xiao Wang felt his chin and thought.

"Abao and Xiao Wang are right. On the premise of not changing the observable behavior of the software, we will continue to refactor in small steps to ensure that the software is in a releaseable state at any time. This means that we can refactor at any time. The simplest refactoring, such as the one I demonstrated just now, does not take a few minutes, and the longest refactoring should not exceed a few hours."

"Let me add one more point." Dabao said: "We must never ignore automated testing. Only automated testing can ensure that the observable behavior of the software is not changed during the refactoring process. This may seem inconspicuous, but it is the most important key. Place."

"Abao is right. We must at least ensure that there are unit tests where we refactor and pass the unit tests before it can be considered as a refactoring."

After a pause, Old Geng waited for everyone to understand what he said just now, and then said: "It seems that everyone is beginning to feel the charm of refactoring. Let's finally look at the comparison of this code before and after refactoring."

"Ok, then let's talk about the remaining 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');
}

"Well, this generation seems to be no problem at first glance. It should be used to render the profile interface. But if we look closely, we will find that the renderPerson method and photoDiv have the same implementation, that is, the part of photo.title The logic of this part is always before the execution of the emitPhotoData function, which is a piece of repetitive code."

"Although this is a seemingly innocuous piece of repetitive code, remember that once there is repetitive code, you must be more careful when reading the repetitive code and pay attention to the subtle differences. If you want to modify the repetitive code, you must find Make all the copies to modify, which makes it easy to make mistakes when reading and modifying the code."

"So, we picked this piece of code for refactoring. As usual, I wrote two unit test cases first." Old Geng began to write 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>`);
  });
});

"Let's run the test first to see if our test case can pass." Old Geng pressed the execute shortcut.

"OK, the test passed, remember to submit a Commit , save our test code. Next, we are ready to start refactoring, this function is relatively simple, we can directly move that line of repeated code to the emitPhotoData function. But this time we I still have to demonstrate a refactoring technique with lower risk to prevent errors. "Old Geng finished speaking, and emitPhotoDataNew ctrl c + ctrl v in the copied function body to complete 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');
}

"Then, we replaced the internally called methods of renderPerson and photoDiv emitPhotoDataNew method. If it is safer, it is best to change a function to execute a test case."

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');
}

"After the replacement is complete, execute the test case to see the effect."

"OK, the test passed, indicating that the refactoring did not cause any problems, then the original emitPhotoData safely deleted, and then emitPhotoDataNew is renamed to emitPhotoData , the refactoring is complete!"

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');
}

"After the modification, don't forget to run the test case." Old Geng's action of running the test case after each modification seems to have formed muscle memory.

"OK, the test passed. This refactoring is complete, submit a Commit , and then look at the comparison before and after the modification."

"Let's continue to look at the next bad smell."

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()}`);
}

"Well, this function seems to be used to print the user's blank message. There are not many problems with the implementation details and naming of this function, but there is a bad smell here, that is-too long function."

"The longer the function, the harder it is to understand. And better explanatory power, easier to share, more choices-all supported by small functions."

"There is a trick to identify this bad smell. That is, if you need to spend time browsing a piece of code to figure out what it is doing, then you should distill it into a function and base it on what it does. Its naming."

"Like this function is the kind that takes a while to browse to figure out what it is doing, but fortunately this function still has some comments."

"It's still the old method. Let's write two test cases first." Lao Geng began to 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'
    ]);
  });
});

"Run the test case after writing it."

"The next extraction step is very simple, because the code itself is annotated, we only need to refer to the rhythm of the annotation to extract it, it is still a small step jogging, first adjust the order of function execution, and layer 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()}`);
}

"After the function is layered, run the test case again to prevent the function from being affected in the process of adjusting the sequence... ok, the test passed."

"The function after the adjusted order becomes very simple, and then we can extract it in four steps."

"The first step is to refine the printBanner function, and then 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()}`);
}

"In the process of extracting, we also removed the comment, because it is really unnecessary. The function name is the same as the comment."

"The second step is to refine the calOutstanding function, and 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()}`);
}

"The third step is to refine the recordDueDate function, and 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 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()}`);
}

"The fourth step is to refine the printDetails function, and 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 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);
}

"After the test case passes, don't forget to submit the code."

"Then let's take a look at the refactored printOwing function. The simple four lines of code clearly describe what the function does. This is the charm of small functions!"

"In the final analysis, the key to making small functions easy to understand is good naming. If you can give a function a good name, people who read the code can understand the function's role through the name, without having to look at what is written in it. This can save a lot of time and reduce your time for pair programming." Old Geng looked at Xiao Li and Xiao Wang with a smile.

Xiao Li Xiao Wang looked at each other with a smile, feeling a little embarrassed.

"Let's take a look at the comparison before and after reconstruction."

"Let's continue." Old Geng kept running.

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
)

"At first glance, priceRange is a function of filtering products. If you look closely, you will find that the main comparison is the size comparison between the product.price field and the incoming parameters min and max isOutSide is true , then filter Products outside the price range are listed, otherwise the products within the price range are filtered out."

"At first glance, there are too many parameters for this function, which makes the client caller very confused. Fortunately isOutSide is pretty good, otherwise this function will look more esoteric."

Xiao Li couldn't help but interject: "Every time I call the priceRange function, I have to take a look at the implementation of this function. I always forget the rule of the last parameter."

"My opinion is the same as Xiao Li." Old Geng nodded: "I don't like marking parameters, because they make it difficult to understand what functions can be called and how they should be called. Using such functions, I still You have to figure out what values are available for the marker parameters."

"Since Xiao Li has also discovered this problem, we will start with the isOutSide that we first write two test cases for the existing code." Old Geng began to write the 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 }
    ]);
  });
});

"Run the unit test... well, it can pass. Then we can simplify the parameters. Let's solve the problem mentioned by Xiao Li just now, which is to mark the parameters. We will refine two functions priceRange "

"Let's modify our unit test code first, and modify it in 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 }
    ]);
  });
});

"I priceRange the isOutSide marking parameter of 060c833789e731, and used the priceOutsideRange and priceInsideRange to achieve the original function. At this time, the test case cannot be run because our code has not been changed. Similarly, adjust the code. Into a way that conforms to 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);
}

"After the code adjustment is complete, let's run the test case. Okay, it's passed!"

"Well, after thinking about this, I can take it a step further and priceRange the function of 060c833789e77e further, just 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);
}

"After the disassembly is complete, remember to run the test case... ok, it passed"

"After the test case is passed, you can start preparing for the migration. Replace the original call of priceRange with a new call, and then priceRange function, just 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
)

"After doing this, the original confusing marker parameters were removed and replaced by two functions with clearer semantics."

"Next, we want to continue to do valuable reconstruction of one, that is, the data organized into structures, because it allows the relationships between data items become clear. For example range of min and max always together in the call Use, then these two parameters can be organized into a structure. I first modify my test case to adapt to the latest changes, 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 }
]);

"After the test case is modified, let's 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
)

"After the modification is completed, run our test case and pass it smoothly. Don't forget to submit the code." After that, Lao Geng called Commit .

"This step of refactoring has streamlined a parameter, which is the most direct value of this refactoring. The real significance of this refactoring is that it will lead to deeper changes in the code. Once a new data structure is identified, I can reorganize the behavior of the program to use these structures. What does this sentence mean in actual application? I'll take this case as an example."

"We will find that priceOutSideRange and priceInsideRange are clear enough, but the internal range still takes some time to understand, and range as a structure we just identified, can continue to be refactored, just 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)
)

"Modify the test case and also pass in the Range object, and then run the test case...ok, it passed. After the test passes, submit the code."

"In this way, the priceOutSideRange and priceInsideRange functions are also clearer. At the same time, range is organized into a new data structure that can be used in any calculation interval."

"Let's take a look at the comparison before and after reconstruction."

"We continue."

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 });

"This global.js seems to be used to provide global data. This is one of the most pungent bad smells."

"This platform is used globally, can I change it to another value? Will it cause any problems?" Old Geng asked

Xiao Li quickly said: "This platform cannot be changed. The backend depends on this field to select token . If it is changed, problems will occur."

platform and token in any corner of the code base, and there is no mechanism to detect which piece of code has been modified. This is a global data problem."

"Whenever we see data that may be polluted by the code everywhere, we still need to wrap the global data with a function, at least you can see where it is modified, and start to control access to it, here I will make a simple Package, 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() });

"Next, 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');
    });
});

"In this way, the source object cannot be modified through the object reference, and I control the platform attributes here, and only open the token . Even so, we still have to avoid global data as much as possible, because global data It's one of the most pungent bad smells!" Old Geng's tone increased.

Xiao Li Xiao Wang nodded frantically.

"Let's take a look at the comparison before and after reconstruction."

"Then let's continue."

Mutable Data

function merge(target, source) {
  for (const key in source) {
    target[key] = source[key];
  }
  return target;
}

"This function seems a bit old." Old Geng was a little puzzled.

"This is a tool function I copied from the previous warehouse. It is used to synthesize objects. It has not been changed." Xiao Wang added.

"Well, the problem with this function is that the source object target merge object is modified. The modification of the data often leads to unexpected results and difficult-to-find bugs. Looking at the program now, there is no problem with this function, but if Failures only occur in very rare circumstances, and it is even more difficult to find out the cause of the failure."

"First write two test cases for verification." Old Geng began to write the 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'
    });
  });
});

"Run it... The second use case reported an error."

"The reason for the error is that the source object was modified and adjusted, which affected the baseConfig . Next, let's adjust the merge function. Now javascript has a very simple way to modify this function."

function merge(target, source) {
  return {
    ...target,
    ...source
  }
}

"After the modification is completed, run the use case again, and you can see that the use case has passed."

"My refactoring method just now actually has a whole software development genre-functional programming, which is completely based on the concept of "data never changes": if you want to update a data structure, return a new copy of the data, The old data remains unchanged, which can avoid many problems caused by data changes."

"The method of encapsulating variables used in the introduction of global data just now is also a common solution to the bad smell of variable data. Also, if the value of variable data can be calculated in other places, this is a solution. A particularly pungent and bad smell. Not only does it cause troubles, bugs and overtime, it is unnecessary."

“I’m not going to expand here. If you two are interested, you can read the book "Refactoring: Improving the Design of Existing Code (Second Edition)". The bad smell I just mentioned is in the book. Both."

Xiao Li Xiao Wang struggled with his pen and wrote down the title of the book.

"Let's take a look at the comparison before and after reconstruction."

"Then let's continue."

Divergent Change

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);

"This function is our earliest refactored function. Its responsibility is to calculate the basic price-quantity discount + shipping. Let's take a look at this function. If the basic price calculation rules change, you need to modify this function. If the discount rules change, you also need to Modifying this function, in the same way, the freight calculation rules will also cause it to change."

"If a module often changes in different directions for different reasons, divergent changes will appear."

"The test cases already exist, so we can directly refactor the function." Old Geng began to write 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);

"After the modification is complete, we run the test case we wrote before... the test passed."

"After the modification, the three functions only need to care about the changes in the direction of their own responsibilities, instead of one function focusing on changes in multiple directions. Moreover, the unit test granularity can be written a little bit more finely, so as to improve the efficiency of troubleshooting. Great improvement."

Dabao added a sentence at the right time: "In fact, this is the single responsibility principle in the object-oriented design principles."

is right, keep simple, 160c833789eee9 only cares about one context at a time, which has always been very important."

"Let's take a look at the comparison before and after reconstruction."

"We continue."

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;
}

"The next thing I want to divergent change- shotgun modification. It takes a while to find a case in the software. Here I will directly take an example from the original book "Refactoring" to explain. "

"In fact, this problem is a bit similar to duplicate code. Duplicated code often causes shotgun modifications."

"Like the demo code above, if reading is changed, the modification of this part of the logic needs to be adjusted across several files."

"If every time you encounter a certain change, you have to make many small changes in many different classes or files, the bad smell you face is shotgun modification. If the code that needs to be modified is scattered around, it is not only difficult to find They are also easy to miss an important modification."

"Here I am here to refactor this code. Since the source code is not very complete, I will only mention the idea of modification here, instead of writing the test code." Old Geng began to write the 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 });

"After the modification is completed, all reading is put together and managed, and I have another advantage after combining it into a class. That is, the class can clearly provide a common environment for these functions, in the object Calling these functions internally can save a lot of parameters, thereby simplifying function calls, and such an object can be more easily passed to other parts of the system."

"If you encounter the problems I just mentioned in the coding process, it is a bad smell. You can refactor with similar refactoring techniques next time. Of course, don't forget to write test cases. "Old Geng said to Xiao Li.

Xiao Li Xiao Wang nodded frantically.

"Let's take a look at the comparison before and after reconstruction."

"Then let's continue."

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;
  }
}

"This code is the account Account and the account type AccountType . If the account type is vip , the loan amount loanAmount will be 20000, otherwise it will only be 10000."

"When obtaining the loan amount, Account internal loanAmount method and another class AccountType internal data exchange particularly frequent, far better than their own internal communication module is located, which is typically Attachment Hunger."

"Let's write two test cases first." Old Geng began to write 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);
  });
});

"The test case can be run directly... ok, it passed."

"Next, we moved 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;
    }
  }
}

"After the move is completed, loanAmount accesses the data of its own module and no longer attaches to other modules. Let's run a test case."

"OK, the test passed, don't forget to submit the code." Old Geng submitted a commit.

"Let's take a look at the comparison before and after reconstruction."

"Let's move on to the next one."

Data Clumps

class Person {
  constructor(name) {
    this._name = name;
  }

  get name() {
    return this._name;
  }

  set name(arg) {
    this._name = arg;
  }

  get telephoneNumber() {
    return `(${this.officeAreaCode}) ${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 ([phone]

"This Person class records the user's name (name), telephone area code (officeAreaCode) and telephone number (officeNumber), there is a bad smell that is not very pungent."

"If I officeNumber field, then officeAreaCode loses its meaning. This means that these two fields always appear together. Except for the Person type, a combination of these two fields will also appear where the phone number is used."

"This bad smell is called a data group, which is mainly reflected in the fact that data items like to stay together in groups. You can often see the same three or four items of data in many places: the same fields in two classes, many functions The same parameters in the signature, these data that always appear together should really have their own objects."

"Old rules, let's write two test cases first." Lao Geng began to write code

describe('test Person', () => {
  test('person.telephoneNumber should return ([phone] 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('([phone]');
  });

  test('person.telephoneNumber should return ([phone] 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('([phone]');
  });
});

"Run the test case... ok, the test passed, ready to start refactoring."

"Let’s create a new TelephoneNumber class to decompose the responsibilities of the Person

class TelephoneNumber {
  constructor(areaCode, number) {
    this._areaCode = areaCode;
    this._number = number;
  }

  get areaCode() {
    return this._areaCode;
  }

  get number() {
    return this._number;
  }

  toString() {
    return `(${this._areaCode}) ${this._number}`;
  }
}

"At this time, let's adjust 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);
  }
}

"The refactoring is complete, we run the test code."

"The test case has passed, don't forget to submit the code."

"I chose to create a new class here instead of a simple record structure, because once you have a new class, you have the opportunity to make the program emit a fragrance. After you get the new class, you can start looking for other bad smells. , Such as "attachment plot", which can help you point out the various behaviors that can be moved to the new class. This is a powerful motivation: useful classes are created, a lot of duplication is eliminated, subsequent development is accelerated, and the original The data mud group finally exerted its full value in their small societ