Open scheibo opened 5 years ago
This should probably throw instead of creating a placeholder.
Originally posted by @Zarel in https://github.com/Zarel/Pokemon-Showdown/pull/5475
The simple way to check this is to change this to throw
and run a bunch of smoke tests to see what happens.
However, I don't know that we can make getEffectByID
throw
if not found - its called by getEffect
, which seems like its supposed to be able to handle arbitrary input? At least, thats the reason you gave on #5194 for not allowing me to skip the hasOwnProperty
call.
I thought getEffect
would manually create an Effect object if not found, rather than delegating to getEffectByID
.
I thought
getEffect
would manually create an Effect object if not found, rather than delegating togetEffectByID
.
For that to happen, we'd want there to be a third, private method which is called internally by getEffect
and getEffectByID
and returns null
/undefined
/sentinel such that getEffect
can then construct a placeholder and getEffectByID
can throw
, but this seems unnecessary (adding a catch
to getEffect
to avoid the third method is a nonstarter).
The biggest reason we don't just return
undefined
is because https://github.com/tc39/proposal-optional-chaining is still Stage 1 after approximately eight years of literally everyone asking for it.
Originally posted by @Zarel in https://github.com/Zarel/Pokemon-Showdown/pull/5475
If we want optionals, I'd prefer an Optional
wrapper class with Optional.of(effect)
or Optional.empty()
because then you only new
up a small object instead of the whole shebang, but I guess that defeats your purpose of the placeholder object making it so you dont need to care the object doesnt exist. With Typescript as a null
or undefined
checker though, you basically get all the same benefits of @NonNull
/@Nullable
checking, which makes returning undefined
safe AFAICT (just less convenient, because without chaining the compiler will force you to handle missing values differently).
Though can we just return nullObject
instead of a named {exists: false}
placeholder? You still get a well behaving Effect
and don't waste cycles constructing a garbage object. The only downside is that obviously the name
wont match - how much does that matter here? Really, my main concern with this 'placeholder pattern' is that it is substantially less efficient than the traditional null object pattern you were going for given how relatively expensive it is to make the placeholder.
I'm fine with a single cached placeholder as a stopgap. But we might want to actually fix it.
throw
ing might be appropriate for all getEffect
calls? The only one is when it's used by stuff like /data
, which we might want to manually wrap in a try-catch?
Context: https://github.com/Zarel/Pokemon-Showdown/pull/5475#discussion_r281000445