diia-open-source / be-pkg-utils

European Union Public License 1.2
14 stars 10 forks source link

Terrible capitalize implementation #7

Open tshemsedinov opened 4 months ago

tshemsedinov commented 4 months ago

https://github.com/diia-open-source/be-pkg-utils/blob/e969374fb769e59ab6bbdb709caeba0199e14a8c/src/applicationUtils.ts#L186-L206

  1. Why it supports multiple different delimiter symbols in a single name? Names should be consistent.
  2. Bad place to use forEach and mutating outer variable from callback
  3. Two calls of capitalizeFirstLetter is not needed
  4. Delimiters detection with filter also looks ugly
  5. Function will not only capitalize first letters it will lower all others
  6. Using regexp here is an obvious overhead
  7. Instead of if (foundDelimiters.length) { this construction better use return-earlier pattern
vird commented 4 months ago

Since private static readonly nameDelimiters: string[] = [' ', '-']

static capitalizeName(name: string): string {
  return name.split(/([- ])/g).map(ApplicationUtils.capitalizeFirstLetter).join('')
}

Pros: Much more readable Cons: Will call ApplicationUtils.capitalizeFirstLetter for delimiter with no effect But for fixing that it will look much more complex

static capitalizeName(name: string): string {
  const splitList = name.split(/([- ])/g);
  for(let i=0,len=splitList.length;i<len;i+=2) {
    splitList[i] = ApplicationUtils.capitalizeFirstLetter(splitList[i])
  }
  return splitList.join('')
}
tshemsedinov commented 4 months ago

It is a good case for .map I suppose

vird commented 4 months ago

https://github.com/diia-open-source/be-pkg-utils/pull/29