Code Smell 21 - Anonymous Functions Abusers

Functions, lambdas, closures. So high order, nondeclarative, and hot.

TL;DR: Don't abuse closures and functions. Encapsulate them into objects.

Problems

• Maintainability

  • Testability
  • Code Reuse
  • Implementation Hiding
  • Debugging

Solutions

1. Wrap functions/closures

2. Reify algorithms in method object / Strategy

Sample Code

Wrong

sortFunction = function(arr, fn) {
  var len = arr.length;

  for (var i = 0; i < len ; i++) {
    for(var j = 0 ; j < len - i - 1; j++){  
      if (fn(arr[j], arr[ j+ 1])) {
        var temp = arr[j];
        arr[j] = arr[j+1];
        arr[j + 1] = temp;
      }
    }
  }
  return arr;
}

scores = [9, 5, 2, 7, 23, 1, 3];  
sorted = sortFunction(scores, (a,b) => {return a > b});

Right

class ElementComparator{
  greatherThan(firstElement, secondElement){
    return firstElement > secondElement;
    //This is just an example. With more complex objects this comparison might not be trivial
  }
}

class BubbleSortingStrategy {
  //We have a strategy, we cant unit test it, change for a polymorphic,
  //Swap and benchmark algorithms etc.
  constructor(collection, comparer){
    this._elements = collection;
    this._comparer = comparer;
  }
  sorted(){ 
    for (var outerIterator = 0; outerIterator < this.size() ; outerIterator++) {
      for(var innerIterator = 0 ; innerIterator < this.size() - outerIterator - 1; innerIterator++){  
        if (this._comparer.greatherThan(this._elements[innerIterator], this._elements[ innerIterator + 1])) {
          this.swap(innerIterator);  
        }
      } 
    } 
    return this._elements; 
  }
  size() {
    return this._elements.length;
  }

  swap(position){    
    var temporarySwap = this._elements[position];
    this._elements[position] = this._elements[position + 1];
    this._elements[position + 1] = temporarySwap;
  }
} 

scores = [9, 5, 2, 7, 23, 1, 3]; 
sorted = new BubbleSortingStrategy(scores,new ElementComparator()).sorted();

Detection

• Closures and anonymous functions are very useful to model code blocks, promises etc. So It'd difficult to tear them apart.

Tags

• Primitive

• Abuser

Conclusion

Humans read code. Software works ok with anonymous functions, but maintainability is compromised when multiple closures are invoked.

Relations

Code Smell 06 - Too Clever Programmer

Maxi Contieri ・ Oct 25 '20

#codenewbie #tutorial #beginners

Credits

Photo by Roman Mager on Unsplash

---

Object-oriented programming increases the value of these metrics by managing this complexity. The most effective tool available for dealing with complexity is abstraction. Many types of abstraction can be used, but encapsulation is the main form of abstraction by which complexity is managed in object-oriented programming.

Rebecca Wirfs-Brock

Software Engineering Great Quotes

Maxi Contieri ・ Dec 28 '20

#codenewbie #programming #quotes #software

---

This article is part of the CodeSmell Series.

How to Find the Stinky parts of your Code

Maxi Contieri ・ May 21 '21

#codenewbie #tutorial #codequality #beginners

Last update: 2021/07/03

Comments

[Magne]

Given that "Humans read code", as you said, I find the first example wayyy easier to read than the second. The second example has over 2x as many lines and over 3x as many characters... I don't know if introducing classes was the right solution here, as it adds indirection and complexity. To me, it thus seems that Right is Wrong and Wrong is Right.

To modify the first example so you solve your 5 stated problems, you could simply:

1. Rename sortFunction() → bubbleSort().

2. Inside it (the sort function), rename fn → compare, for clarity, so the if sentence reveals the intent: if (compare(arr[j], arr[j+1]))

3. Name the anonymous function before passing it into the (newly renamed) sort function:
   

const compare = (a,b) => { return a > b };
sorted = bubbleSort(scores, compare);

I do agree that anonymous functions can and often are abused, though. But mostly because they are often injected everywhere (typically obfuscating param lists), without the developer having taken the the time to name and declare it (which would simplify the params list, and also reveal the function's intent better).

[Valentin Nechayev]

+100.
Maybe the problem exists (I canʼt say on its real spread), but the example shows quite opposite case.

[Maxi Contieri]

Why is the opposite?
can you elaborate?

Do you have a better example?

[Valentin Nechayev]

Why is the opposite?

"Wrong" code in your post is, except naming, is definitely better than "right" code. The only aspect to correct is naming, as already said in another comments (and Iʼd doubt whether naming was spoiled unintentionally).

Do you have a better example?

Should I? Itʼs your goal to show the initial thesis by good examples. But, to cast a seed for discussion, a better example would show
1) effect of passing an unknown function through multiple execution levels and possibly stored in a data structure (and reused much later on),
2) consequence of absence of traceable function origin (and, what is important, just function name isn't enough).

[Maxi Contieri]

IMHO "Wrong" code is more cryptic and less declarative

Even it might seem more compact it is programmed like the 50s

On the contrary, "Right" code is higher level, more reusable and more declarative

[Magne]

I respectfully disagree, and can only refer you to my first comment for my reasoning, in case you overlooked it.

[𒎏Wii 🏳️‍⚧️]

Problems

• Maintainability
• Testability
• Code Reuse
• Implementation Hiding
• Debugging

1. That's very subjective. To one developer might find it easier to maintain an object-oriented codebase, while others might prefer working on functional code.
2. Normally, an anonymous function is tested indirectly along with the surrounding code.
3. Re-using an anonymous function is trivial in most cases: just extract it into a named function when you find yourself needing it a second or third time.
4. There's two interfaces in your first example:
   • the higher order sortFunction
   • presumably the interface of the outer function surrounding the snippets For both of them, the interface is completely separated from its implementation, just as it'd be with an object. If you extract the comparator function in a later refactoring step, it will also share this property.
5. There's no real difference between a debugger telling you "error in function X" and "error in anonymous function on line Y in file Z".

[JoelBonetR 🥇]

The "Right" code you show seems OOP hell to me.
Using Objects mapped to "real world things" (laughs in flags, nulls or a sea of properties, getters, setters and methods to overcome the first two) for absolutely everything is a nonsense and the reason for FPP to take over those recent years.

To use objects as data structures is OK (is what they are in first place). But we're sorting an array here, which is by definition a data structure.

You don't need to pass this fn as param either:

function bubbleSort(arr){   
  let i, j, len = arr.length, isSwapped = false;

  for(i = 0; i < len; i++){  
    isSwapped = false;  

    for(j = 0; j < len; j++){
        if(arr[j] > arr[j + 1]){
          let temp = arr[j];
          arr[j] = arr[j+1];
          arr[j+1] = temp;
          isSwapped = true;
        }
    }
    if (!isSwapped) break;
  }
  return arr;
}