Reduce the cyclomatic complexity of a typescript method

Question

Provided is the typescript function, where we need to reduce the cyclometric complexity. I am thinking off to provide the inverted if but that doesn't help much.

  updateSort(s: Sort) {
    if (s.active !== this.personSort.active || s.direction !== this.personSort.direction) {
      let sortFunc: (a: PersonInfo, b: PersonInfo) => number;
      switch (s.active) {
        default:
        case 'personName':
          sortFunc = (a, b) => (a.name).localeCompare(b.name);
          break;
        case 'personAddress':
          sortFunc = (a, b) => (a.pageTypeDescription.toLocaleString()).localeCompare(b.pageTypeDescription.toLocaleString());
          break;
        case 'personVisitors':
          sortFunc = (a, b) => a.viewsCount - b.viewsCount;
          break;
        case 'personContacts':
          sortFunc = (a, b) => a.clicksCount - b.clicksCount;
          break;
      }

      if (s.direction === 'desc') {
        this.sliceList.sort((a, b) => sortFunc(b, a));
      } else {
        this.sliceList.sort(sortFunc);
      }
    }
  }

you could make a separate method `getSortFunc(s: Sort){...}` containing your switch — Andrei, Sep 19 '22 at 18:02
Might be better suited for [code review](https://codereview.stackexchange.com/)? — kelsny, Sep 19 '22 at 18:19

zapl · Answer 1 · 2022-09-19T23:21:33.417

As noted in the comments, you can invert the boolean logic to return early, which is always nicer to read and you can easily extract the switch statement into it's own function. E.g.

updateSort(s: Sort) {
    if (s.active === this.personSort.active
        && s.direction === this.personSort.direction) {
        return
    }

    const sortFunc = this.getSortFunc(s.active)
    if (s.direction === 'desc') {
        this.sliceList.sort((a, b) => sortFunc(b, a))
    } else {
        this.sliceList.sort(sortFunc)
    }
}

getSortFunc(type: string): (a: PersonInfo, b: PersonInfo) => number {
    // no need to repeat that as well
    const localeCompare = (a: string, b: string) => a.localeCompare(b)
    switch (type) {
        default:
        case 'personName':
            return (a, b) => localeCompare(a.name, b.name)
        case 'personAddress':
            return (a, b) =>
                localeCompare(
                    a.pageTypeDescription.toLocaleString(),
                    b.pageTypeDescription.toLocaleString()
                )
        case 'personVisitors':
            return (a, b) => a.viewsCount - b.viewsCount
        case 'personContacts':
            return (a, b) => a.clicksCount - b.clicksCount
    }
}

and an example of getting rid of the switch entirely like noted by @caTS could be

localeCompare = (a: string, b: string) => a.localeCompare(b)

sortsFunctions: Record<string, (a: PersonInfo, b: PersonInfo) => number> = {
    personName: (a, b) => this.localeCompare(a.name, b.name),
    personAddress: (a, b) =>
        this.localeCompare(
            a.pageTypeDescription.toLocaleString(),
            b.pageTypeDescription.toLocaleString()
        ),
    personVisitors: (a, b) => a.viewsCount - b.viewsCount,
    personContacts: (a, b) => a.clicksCount - b.clicksCount,
}

getSortFunc(type: string): (a: PersonInfo, b: PersonInfo) => number {
    return this.sortsFunctions[type] ?? this.sortsFunctions.personName
}

There's more you could do, similar to 3limin4t0r you could also refactor into

// fully generic part
type Comparator<T> = (a: T, b: T) => number
const numCompare: Comparator<number> =
    (a, b) => a - b
const localeCompare: Comparator<string> =
    (a, b) => a.localeCompare(b)
const invert = <T>(comparator: Comparator<T>): Comparator<T> =>
    (a, b) => comparator(b, a)
const compareProperties = <T, P>(propExtract: (object: T) => P, comparator: Comparator<P>): Comparator<T> =>
    (a, b) => comparator(propExtract(a), propExtract(b))

// specific to the task
const sortFunctions: Record<string, Comparator<PersonInfo>> = {
    personName: compareProperties(p => p.name, localeCompare),
    personAddress: compareProperties(p => p.pageTypeDescription.toLocaleString(), localeCompare),
    personVisitors: compareProperties(p => p.viewsCount, numCompare),
    personContacts: compareProperties(p => p.clicksCount, numCompare),
}
const getSortFunc = (type: string) => sortFunctions[type] ?? sortFunctions.personName

const sortsEquals = (a: Sort, b: Sort) => a.active === b.active
                                          && a.direction == b.direction

class PersonPage {
    personSort: Sort
    sliceList: PersonInfo[]
    updateSort(s: Sort) {
        if (sortsEquals(s, this.personSort)) {
            return
        }
        let sortFunc = getSortFunc(s.active)
        if (s.direction === 'desc') {
            sortFunc = invert(sortFunc)
        }
        this.sliceList.sort(sortFunc)
    }
}

Yep. A factory function clears this up a lot. I was about to submit an example and I realize mine is identical to yours. Nice work. — Justin Gilman, Sep 19 '22 at 20:41

3limin4t0r · Answer 2 · 2022-09-20T09:23:53.280

I would personally extract a lot of the basic sorting stuff into their own general purpose helpers. This would reduce your method to:

updateSort(s: Sort) {
  if (s.active === this.personSort.active && s.direction === this.personSort.direction) return;
  
  const direction = s.direction === "desc" ? desc : asc;
  this.sliceList.sort(direction((personInfo: PersonInfo) => {
    switch (s.active) {
      case "personAddress":
        return personInfo.pageTypeDescription.toLocaleString();
      case "personVisitors":
        return personInfo.viewsCount;
      case "personContacts":
        return personInfo.clicksCount;
      default:
        return personInfo.name;
    }
  }));
}

This does require you to introduce the asc and desc helper functions. These functions take a function argument fn and returns an ascending/descending comparator. When the comparator is invoked, fn is applied to both a and b before they are passed to the comparator.

This allows us to remove a lot of the code "duplication".

// "duplicate" code
 (a.pageTypeDescription.toLocaleString()).localeCompare(b.pageTypeDescription.toLocaleString())
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Here are the general purpose helper functions as I've defined them for this solution:

// Returns true if item has the given method, false otherwise.
function hasMethod(item: any, methodName: string | symbol): boolean {
  // intentional != usage, hits both null and undefined
  return item != null && typeof item[methodName] === "function";
}

// Compares 2 items. Returns a negative number if A < B, returns a
// positive number if A > B, returns 0 otherwise. If both A and B
// have the method `localeCompare` then `a.localeCompare(b)` is
// used instead.
function compare(a: any, b: any): number {
  const isLocaleComparable = hasMethod(a, "localeCompare") && hasMethod(b, "localeCompare");
  if (isLocaleComparable) return a.localeCompare(b);
  
  return -(a < b) || +(a > b);
}

// Returns an ascending comparator that applies fn to both A and B
// before comparing the resulting values with each other.
function asc(fn: (item: any) => any) {
  return function (a: any, b: any): number {
    return compare(fn(a), fn(b));
  };
}

// Returns an descending comparator that applies fn to both A and B
// before comparing the resulting values with each other.
function desc(fn: (item: any) => any) {
  return function (a: any, b: any): number {
    return compare(fn(b), fn(a));
  };
}

Of course you're free to customize them further to fit your needs.

Reduce the cyclomatic complexity of a typescript method

2 Answers2