Expression is already of boolean type

[tshemsedinov]

be-pkg-utils/src/applicationUtils.ts

Lines 178 to 184 in e969374

	static isItnFormatValid(itn: string): boolean {
	if (/^0{1,20}$/.test(itn) || itn === 'null' || !/^\d{10}$/.test(itn)) {
	return false
	}
	return true
	}

static isItnFormatValid(itn: string): boolean {
  return !/^0{1,20}$/.test(itn) && itn !== 'null' && /^\d{10}$/.test(itn);
}

Also it will be better to remove null comparison and use string operations like .startsWith, .contains and so on in such simple cases at least we do not need regexp.

[vird]

Problem with this code is not only with "already boolean", it has redundant logic

According to tests this code can be rewritten in much simplier way

be-pkg-utils/tests/unit/applicationUtils.spec.ts

Lines 813 to 819 in e969374

	it('should return false for an ITN consisting of all zeroes', () => {
	const itn = '0000000000'
	const result = ApplicationUtils.isItnFormatValid(itn)
	expect(result).toBe(false)
	})

• should return false for an invalid ITN format
  if (!/^\d{10}$/.test(itn)) return false
• should return false for an ITN consisting of all zeroes
  Then if (!/^0*$/.test(itn)) return false
  This one also covers should return false for the string "null"

So

static isItnFormatValid(itn: string): boolean {
  // valid format should contain 10 digits, no letters allowed
  if (!/^\d{10}$/.test(itn)) return false
  // edge case. We don't allow all digits to be 0
  if (/^0*$/.test(itn)) return false
  return true
}

So check for null can be removed

[Deimos308]

@vird last condition can also be simplified to:

static isItnFormatValid(itn: string): boolean {
  // valid format should contain 10 digits, no letters allowed
  if (!/^\d{10}$/.test(itn)) return false
  // edge case. We don't allow all digits to be 0
  return +itn !== 0
}

No additional RegExp is required.

[vird]

Last condition should be not simplified
Reason. If I want to remove part of logic I need remove 1 line
In your case I can't remove last line

[Deimos308]

It sounds strange to me in several ways:

1. Using an extra if condition + a regular expression where it can be very easy to skip it.
2. "If I want..." - sounds like premature optimization in order to have a "universal solution", but what's the point?
   The function is as simple as possible...
3. "In your case I can't remove last line", do we need it?:

   ...
   // edge case. We don't allow all digits to be 0
   return +itn !== 0
}

  ...
  return true
}

Moreover, in this case I would personally prefer the format with a single line format provided by @tshemsedinov :

   ...
   // valid format should contain 10 digits, no letters allowed
   // edge case. We don't allow all digits to be 0
   return !/^\d{10}$/.test(itn) && +itn !== 0
}

Perhaps I don't fully understand your position... Can you explain with an example please?

[vird]

• v8 will optimize last condition
• my solution is more readable
• my solution is more editable