Refactoring to nested functions inside methods (JavaScript)

Question

I've been reading Refactoring (2nd) by Martin Fowler. In the first chapter, he shows an example of refactoring a function where he extracts other functions from it and places them inside that function as nested functions.

Specifically, starting from this function:

function statement(invoice, plays) {
 let totalAmount = 0;
 let volumeCredits = 0;
 let result = `Statement for ${invoice.customer}\n`;
 const format = new Intl.NumberFormat("en-US",
 {
 style: "currency", currency: "USD",
 minimumFractionDigits: 2
 }).format;
 for (let perf of invoice.performances) {
 const play = plays[perf.playID];
 let thisAmount = 0;
 switch (play.type) {
 case "tragedy":
 thisAmount = 40000;
 if (perf.audience > 30) {
 thisAmount += 1000 * (perf.audience - 30);
 }
 break;
 case "comedy":
 thisAmount = 30000;
 if (perf.audience > 20) {
 thisAmount += 10000 + 500 * (perf.audience - 20);
 }
 thisAmount += 300 * perf.audience;
 break;
 default:
 throw new Error(`unknown type: ${play.type}`);
 }
 volumeCredits += Math.max(perf.audience - 30, 0);
 if ("comedy" === play.type) volumeCredits += Math.floor(perf.audience / 5);
 result += ` ${play.name}: ${format(thisAmount / 100)} (${perf.audience} seats)\n`;
 totalAmount += thisAmount;
 }
 result += `Amount owed is ${format(totalAmount / 100)}\n`;
 result += `You earned ${volumeCredits} credits\n`;
 return result;
}

he arrived at this function:

function statement(invoice, plays) {
 let result = `Statement for ${invoice.customer}\n`;
 for (let perf of invoice.performances) {
 result += ` ${playFor(perf).name}: ${usd(amountFor(perf))} (${perf.audience} seats)\n`;
 }
 result += `Amount owed is ${usd(totalAmount())}\n`;
 result += `You earned ${totalVolumeCredits()} credits\n`;
 return result;
 function totalAmount() {
 let result = 0;
 for (let perf of invoice.performances) {
 result += amountFor(perf);
 }
 return result;
 }
 function totalVolumeCredits() {
 let result = 0;
 for (let perf of invoice.performances) {
 result += volumeCreditsFor(perf);
 }
 return result;
 }
 function usd(aNumber) {
 return new Intl.NumberFormat("en-US", {
 style: "currency",
 currency: "USD",
 minimumFractionDigits: 2
 }).format(aNumber / 100);
 }
 function volumeCreditsFor(aPerformance) {
 let result = 0;
 result += Math.max(aPerformance.audience - 30, 0);
 if ("comedy" === playFor(aPerformance).type) result += Math.floor(aPerformance.audience / 5);
 return result;
 }
 function playFor(aPerformance) {
 return plays[aPerformance.playID];
 }
 function amountFor(aPerformance) {
 let result = 0;
 switch (playFor(aPerformance).type) {
 case "tragedy":
 result = 40000;
 if (aPerformance.audience > 30) {
 result += 1000 * (aPerformance.audience - 30);
 }
 break;
 case "comedy":
 result = 30000;
 if (aPerformance.audience > 20) {
 result += 10000 + 500 * (aPerformance.audience - 20);
 }
 result += 300 * aPerformance.audience;
 break;
 default:
 throw new Error(`unknown type: ${playFor(aPerformance).type}`);
 }
 return result;
 }
}

Looks simple, but now my question is how to make this refactoring if the statement function is a method of a class instead, and many of the extracted functions need to call instance properties through this.propertyName?

In this case, we can't extract them into nested functions because this in a nested function could refer to a function, not an instance of the object. We can create an arrow function and assign it to the constant in which the meaning of this will be preserved, but then such functions would not be hoisted, and we'd need to declare them at the beginning of the statement method, which would be ugly.

So, should we extract all those extra functions into separate (private) methods of this class or is there a better way?

Let's use an example to illustrate this. Imagine we have a simplified class:

class StatementGenerator {
 constructor(plays) {
 this.plays = plays;
 }
 public statement(invoice) {
 let totalAmount = 0;
 for (let perf of invoice.performances) {
 const play = this.plays[perf.playID];
 // Some business logic
 }
 return `Amount owed is ${format(totalAmount / 100)}\n`;
 }
}

And then want to refactor this line into it's own function/method:

const play = this.plays[perf.playID];

We can either extract it into a method:

class StatementGenerator {
 constructor(plays) {
 this.plays = plays;
 }
 public statement(invoice) {
 let totalAmount = 0;
 for (let perf of invoice.performances) {
 const play = this.playFor(perf);
 // Some business logic
 }
 return `Amount owed is ${format(totalAmount / 100)}\n`;
 }
 
 private playFor(aPerformance) {
 return this.plays[aPerformance.playID]
 }
}

Or to extract it into a nested arrow function:

class StatementGenerator {
 constructor(plays) {
 this.plays = plays;
 }
 public statement(invoice) {
 const playFor = (aPerformance) => {
 return this.plays[aPerformance.playID]
 }
 let totalAmount = 0;
 for (let perf of invoice.performances) {
 const play = playFor(perf);
 // Some business logic
 }
 return `Amount owed is ${format(totalAmount / 100)}\n`;
 }
}

• candied_orange: "how to make this refactoring if statement is a method of a class" Huh? Is this worded the way you meant? Please consider an edit.
• Greg Burghardt: @candied_orange: I think I understand what the OP is asking about. I changed that sentence. Azera, if this is not what you meant, feel free to revert my edit, but please consider clarifying what you mean if you revert my edit.
• azera: @GregBurghardt, thank you for your edit. This is exactly what I meant.
• 5ar: If you wan't to go the typical OO route of this being your instance reference inside all methods, I advise not using the method syntax for defining methods but fields that you assign arrow functions to (e.g. public statement = (invoice) => ... and so on). With this approach the transpiler will bind all of your methods to the this object at the beginning of the constructor, so you don't need to worry about method order as you had to with const arrow functions because methods are bound to this in runtime.

Answer 1

JavaScript is its own beast here. The this variable changes based on how you call a function, so calling a nested function changes what this points to, which is the heart of your question.

The key difference between the example and your question is programming paradigm: functional versus object-oriented.

If the statement function is a method of a class which needs access to instance state, then nested functions are not beneficial in JavaScript because calling the nested totalAmount() function needs to access this.foo, which now points to the global or window object instead of your class. In this situation, nested functions work best as methods on the same class, so you can maintain consistency in what this points to.

Nested functions allow you to decompose a large "outer" function into smaller functions with good names. This works well in a functional paradigm or OO languages that don't change what this points to based on how a function is called. You accomplish the same decomposition in JavaScript by adding new methods to the same class.

If using nested functions really is the route you want to take, you have no other choice but to pass all instance data as parameters to these nested functions. You get to keep the nested functions at the expense of adding parameters to them. Whether that is a good idea or not is a matter of personal taste — for you and each of your teammates. Might be worth doing this refactoring job and showing it to a teammate or two if you are unsure.

• azera: Sounds clear. Now I'm more inclined to use nested functions in methods only if they don't refer to this. If they do, make them methods of the class. But what do you think of nesting arrow functions in methods? They preserve the meaning of this so we don't have the issue of this pointing to the global object (but at the expense of them not being hoisted).
• azera: I added an example to the main post to better illustrate my point. Would you like to look again?