Code Smells: Long Methods

✅ TL;DR

You should write like a good presenter, use your words wiselly. Follow this rules in your code:

• Extract methods
• Reduce Local Variables and Parameters
• Decompose Conditionals

---

👃 Code Smells

What is a code smell? Code Smells are the traces in the code that indicates a deeper problem in the application or codebase. It's like the cough that you do when you are sick, it's a symptom. For code, the Code Smells are symptoms for a bad or spaghetti code.

One of this symptom is the Long Method problem.

Imagine that you find this code:

---

✂️ Extracting Methods

void printOwing() {
  printBanner();

  // Print details.
  System.out.println("name: " + name);
  System.out.println("amount: " + getOutstanding());
}

For some, this is some ok code, but it attracts problems, and it smells bad. It says to me that everything that I need to print, I can just place it inside the printOwing method. If you see it like it's telling a story, it's like being wordy, it's saying more than it needs.

"When entering printOwing you go to printBanner AND print the name AND THEN print the amount + getOutstading AND more things that you want to add."

When telling a story, you want to be focused on the main acts, so a better way to do this would be writing the code like this:

void printOwing() {
  printBanner();
  printDetails(getOutstanding());
}

void printDetails(double outstanding) {
  System.out.println("name: " + name);
  System.out.println("amount: " + outstanding);
}

"When entering printOwing you go to printBanner AND printDetails"

If the listener wants to know what's in the printBanner or printDetails, he can ask!

The main thing here is to extract the method, much more like being succinct in a conversation.

---

♻️ Reduce Local Variables

If your code requires some variable that's being reused, you should place it in a way it can be reused. It's almost like using synonyms or referencing things implicitly.

double calculateTotal() {
  double basePrice = quantity * itemPrice;
  if (basePrice > 1000) {
    return basePrice * 0.95;
  }
  else {
    return basePrice * 0.98;
  }
}

It's using here the same basePrice, but it's being repetitive. One way to fix it is doing it like this:

double calculateTotal() {
  if (basePrice() > 1000) {
    return basePrice() * 0.95;
  }
  else {
    return basePrice() * 0.98;
  }
}
double basePrice() {
  return quantity * itemPrice;
}

When writing like this, you have a great way to change the basePrice if something is changes. It solidifies the code.

It be applied to objects also, if you see things like

amountInvoicedIn (start: Date, end: Date)
amountReceivedIn (start: Date, end: Date)
amountOverdueIn (start: Date, end: Date)

you can change it to

amountInvoicedIn (date: DateRange)
amountReceivedIn (date: DateRange)
amountOverdueIn (date: DateRange)

This is by introducing a parameter object. But you can also preserve a whole object, like this:

int low = daysTempRange.getLow();
int high = daysTempRange.getHigh();
boolean withinPlan = plan.withinRange(low, high);

boolean withinPlan = plan.withinRange(daysTempRange);

Now, if you have this story that the facts are so intricate you can separate part of the story to a different section of you lore. Translating it to code would be like you can try moving the entire method to a separate object via replace method with method object.

It would look like we have this code and we cannot extract the price method.

class Order {
  // ...
  public double price() {
    double primaryBasePrice;
    double secondaryBasePrice;
    double tertiaryBasePrice;
    // Perform long computation.
  }
}

One solution would be to turn it into this:

class Order {
  // ...
  public double price() {
    return new PriceCalculator(this).compute();
  }
}

class PriceCalculator {
  private double primaryBasePrice;
  private double secondaryBasePrice;
  private double tertiaryBasePrice;

  public PriceCalculator(Order order) {
    // Copy relevant information from the
    // order object.
  }

  public double compute() {
    // Perform long computation.
  }
}

As the refactoring guru tells:
"Transform the method into a separate class so that the local variables become fields of the class. Then you can split the method into several methods within the same class."

---

💡 Decompose Conditional

This feels like the person that talks a whole story to explain possibilities. Like, dude, I just want to know the possibilities, not the whole lore. That's why in the code when we have something like this:

if (date.before(SUMMER_START) || date.after(SUMMER_END)) {
  charge = quantity * winterRate + winterServiceCharge;
}
else {
  charge = quantity * summerRate;
}

we change it to:

if (isSummer(date)) {
  charge = summerCharge(quantity);
}
else {
  charge = winterCharge(quantity);
}

This way we can be concise and more descriptive. So remember to decompose conditionals.

---

📏👍 Rule of thumb

Always remember that in object-oriented code, classes with short methods live longest. The longer a method or function is, the harder it becomes to understand and maintain it. Remember that writing code can be much closer to talking, I mean, so be intentional about your writing.

📚 References:

• refactoring.guru

Comments

[Jon Randy 🎖️]

It's using here the same basePrice, but it's being repetitive. One way to fix it is doing it like this...

The code shown is worse than the original as it needlessly calculates basePrice twice, and also forces whoever is reading the code to look elsewhere for how basePrice is calculated. This reduces readability and performance.

Also, your basePrice function will not work as it doesn't know what quantity and itemPrice are. The earlier printDetails function similarly does not know what name is.

[Matheus Gomes 👨‍💻]

Reading it again I understand that this doesn't seem good. Thanks for this comment, can I add it to the article?