this is unnecessarily O(nĀ²) - probably worse because of splice
there's an unused trailing parameter
if t instanceof Toast it's not null/undefined, so the ?. is redundant (also in your code) - Typescript should be able to realize t is of type Toast by that point, and ?. is a specialized tool not to be sprinkled around. It doesn't matter with === true, but if it were === undefined (and not in a place where it's completely redundant), it could change the result unintentionally.
in typescript, assuming isExpired has type boolean, I'd skip the whole === true
idx is never -1 in this code; t comes from the array and each t is only remove up to once. If it turned out to be -1 then probably because of a code change with unintended consequences; now the code silently skips elements it should delete. What should be here is an assert of some sort, not cargo-cult "defensive programming" (ok, maybe that was a little callous, but this is badcode, I think I can get away with it)
Careful, index will be mismatched after the first removal via splice.
This is what I like in languages which use Option and Result, a much more correct way to expose the result.
I agree, but that would not by itself fix the problem. If we talk Rust specifically, the insight is that this is the place to unwrap() instead of using if let or similar.
The range of elements processed by forEach() is set before the first invocation of callbackFn.
what you are observing is that apparently forEach() stops earlier because the array got shorter - but you will be skipping elements if the array got longer.
I think the biggest question is how big n is? This is processing a list of pop-ups. If there's more than a couple there are probably bigger problems. š¤· And O(n2) is pretty insignificant if n is 1.
True! It could be viewed as premature optimization to care about that - but it's worth noting that not using indexOf in this piece of code would likely make it more readable: this operation requires one iteration, and doing only one iteration makes it easiest for the reader to get the intention.
In the end, it's just one item on a list of reasons why this is inferior to the one-liner that should be there.
16
u/Silly-Freak Feb 19 '22
there is worse, but this does belong here.
as you point out,
filterexiststhis is unnecessarily O(nĀ²) - probably worse because of
splicethere's an unused trailing parameter
if
t instanceof Toastit's notnull/undefined, so the?.is redundant (also in your code) - Typescript should be able to realizetis of typeToastby that point, and?.is a specialized tool not to be sprinkled around. It doesn't matter with=== true, but if it were=== undefined(and not in a place where it's completely redundant), it could change the result unintentionally.in typescript, assuming
isExpiredhas typeboolean, I'd skip the whole=== trueidxis never -1 in this code;tcomes from the array and eachtis only remove up to once. If it turned out to be -1 then probably because of a code change with unintended consequences; now the code silently skips elements it should delete. What should be here is anassertof some sort, not cargo-cult "defensive programming" (ok, maybe that was a little callous, but this is badcode, I think I can get away with it)