In the previous blog posts I wrote about Feature Envy and Divergent Change. We will continue with what I call “Too much indirection”. I am not sure if that is a code smell or more of a code burden.
Indirection is the ability to reference something using a name, reference, or container instead of the value itself. When it is overused though, it can make the really difficult to follow the flow of the code. Imagine the following class:
class Product {
private type: string;
private origin: string;
private category: string;
private stock: number;
constructor(type: string, origin: string, category: string, stock: number) {
this.type = type;
this.category = category;
this.origin = origin;
this.stock = stock;
}
public getBasePrice() : number {
if (this.isEuOrigin()) {
if (this.isLuxuryProduct()) {
return 10;
} else {
if (this.hasMinRequiredStock()) {
return 2;
}
return 5;
}
} else {
if (this.hasMinRequiredStock()) {
return 5;
} else {
if (this.isOnDiscount()) {
return 2;
}
return 7;
}
}
}
private isEuOrigin(): Boolean {
return this.origin === 'eu';
}
private isLuxuryProduct(): Boolean {
return this.category === 'luxurious';
}
private hasMinRequiredStock(): Boolean {
return this.stock > 5;
}
private isOnDiscount(): Boolean {
return this.hasMinRequiredStock() && !this.isLuxuryProduct();
}
}The problem with this code (apart from the nested if statements which is another problem) and all these small (private) functions is that if you want to understand how getBasePrice() works you have to scroll up and down again and again, which needs a lot of cognitive power. In my dummy example I just had 4 functions, imagine having 10. In cases like that I prefer to either inline the statements or declare local variables for them.
class Product {
private type: string;
private origin: string;
private category: string;
private stock: number;
constructor(type: string, origin: string, category: string, stock: number) {
this.type = type;
this.category = category;
this.origin = origin;
this.stock = stock;
}
public getBasePrice() : number {
const isEuOrigin = this.origin === 'eu';
const isLuxuryProduct = this.category === 'luxurious';
const hasMinRequiredStock = this.stock > 5;
const isOnDiscount = hasMinRequiredStock && !isLuxuryProduct;
if (isEuOrigin) {
if (isLuxuryProduct) {
return 10;
} else {
if (hasMinRequiredStock) {
return 2;
}
return 5;
}
} else {
if (hasMinRequiredStock) {
return 5;
} else {
if (isOnDiscount) {
return 2;
}
return 7;
}
}
}
}I prefer to extract a statement to a seperate function only if the same statements is used in another function of the class, otherwise having local function variables makes reading the flow of the code easier.