More refactorings #446
More refactorings #446
Conversation
cli/alsatian-cli-options.ts
Outdated
| @@ -4,6 +4,10 @@ import { InvalidTimeoutValueError } from "./errors/invalid-timeout-value-error"; | |||
| import { MissingArgumentValueError } from "./errors/missing-argument-value-error"; | |||
| import { Unused } from "../core/unused"; | |||
|
|
|||
| function removeItemByIndex(array: Array<any>, index: number) { | |||
There was a problem hiding this comment.
Please put this in another file.
| } | ||
|
|
||
| function outcomesContains(outcomes: Array<TestOutcome>, outcome: TestOutcome) { | ||
| return outcomes.some(o => o === outcome); |
There was a problem hiding this comment.
It looks as though indexOf is faster than some - why not use that?
There was a problem hiding this comment.
More readable I think unless you disagree?
There was a problem hiding this comment.
- I've just benchmarked the function in question on node over a billion iterations ran 10 times shows only a 15% slowdown using some. Perhaps browsers are more affected?
There was a problem hiding this comment.
For context the average time for execution is ~ 160ns (yep nano :) ) I'm not sure we'd manage to gain enough performance for the readability
There was a problem hiding this comment.
I think that arr.indexOf(x) !== -1 is quicker to glance at and see what's going on but that's probably only because it's syntax that I've used for years while arr.some(i => i === x) is fairly new.
Happy to keep some though
There was a problem hiding this comment.
I'd keep some for readability too. Even though indexOf(x) !== -1 has been used for years, I would favour a semantically nicer approach like list.contains(x) currently some is your best bet.
There was a problem hiding this comment.
@7jpsan thanks for you input :) good to see you on here!!
|
Sorry @Jameskmonger forgot to put a note this is still in progress as trying to get feedback from code climate. |
|
@Jameskmonger I think that should do for now - some more refactors available but nailed a lot of the bigger ones so everything should be mainly an "A" now :) |
|
Happy with this one @Jameskmonger ? |
|
LGTM @jamesrichford. I can't merge due to codeclimate. |
|
@Jameskmonger thanks - should all be automatic checks from now on :) |
Description
Make more improvements recommended by code climate
Checklist
npm testto make sure everything else still worksnpm run reviewto ensure the code adheres to the repository standards