The ternary condition in the pluralize function in text-utils is unnecessary since its "else" condition is unreachable. Because of the order of the cases in pluralize, if the maxNumberOfNouns === 1 conditional is entered, nouns.length will always be greater than 1, since the prior two conditionals check if nouns.length equals 0 or 1. Therefore, it can be removed.
Details
Details
The length property of an array can never be a negative number, and only a positive integer greater than or equal to 0. So nouns.length is guaranteed to be greater than 1 if the maxNumberOfNouns === 1 conditional is reached.
In addition, after @palys-swm's review, I checked all instances of lib/text/text-utils (specifically calls to pluralize and pluralizeAndTrim) in the codebase to make sure this change did not break anything:
- Test to make sure InlineSidebars are displayed correctly:
Also, to be doubly sure, this is the usage of pluralizeAndTrim in InlineSidebarText, which calls pluralizeAndTrim with maxNumberOfNouns = 25. The code that was changed in this diff was in the conditional, which is only executed if maxNumberOfNouns === 1, so this change will not affect InlineSidebar.
- Test to make sure JoinThreadMessageSpec works correctly:
This is the only call to pluralize/usage of text-utils in JoinThreadMessageSpec:
Because this call to pluralize does not define the maxNumberOfNouns parameter, the code that was changed will never be executed since maxNumberOfNouns is by default initialized to 3 (see line 5 in this diff), not 1. The code that was changed in this diff was in the conditional, which is only executed if maxNumberOfNouns === 1, so this change will not affect JoinThreadMessageSpec.
- Test to make sure thread-utils works correctly:
This is the only call to pluralize in thread-utils:
Once again, because this call to pluralize does not define the maxNumberOfNouns parameter, the code that was changed will never be executed since maxNumberOfNouns is by default initialized to 3, not 1. Therefore, this change will not affect thread-utils (or anything that uses it).
- Test to make sure LeaveThreadMessageSpec works correctly:
Same logic as described above for JoinThreadMessageSpec:
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/utils/text-utils.js | ||
|---|---|---|
| 20–21 | This change is to match the later style in trimText of removing the else branch if the if branch contains a return statement. I think this is good style (re: no-else-after-return), although I know @ashoat prefers to have else-ifs for chains of if blocks that have returns. But since this is just an if-else, there doesn't need to be an else block. | |
Comment Actions
This change definitely needs a test plan. For example, you can simply check if inline sidebars are displayed correctly. Also, this is a great function to have some tests.