Fix handling of null and undefined in EdgeMap #65
Conversation
|
|
||
| // @NotNull | ||
| put(key: number, /*@Nullable*/ value: T): EdgeMap<T>; | ||
| put(key: number, value: T): EdgeMap<T>; |
There was a problem hiding this comment.
📝 The only cases where a "nullable" value was passed to this method are in ArrayEdgeMap. Rather than make the parameter T | undefined here, I updated the argument in ArrayEdgeMap.put to allow those cases specifically when working with the concrete implementation ArrayEdgeMap.
| @@ -49,15 +49,15 @@ export class SingletonEdgeMap<T> extends AbstractEdgeMap<T> { | |||
| this.value = value; | |||
| } else { | |||
| this.key = 0; | |||
| this.value = null; | |||
| this.value = undefined; | |||
There was a problem hiding this comment.
💭 I know the value is already undefined. Does it help clarity to keep this assignment here and/or do we gain a performance benefit by removing the assignment?
| @@ -88,17 +88,17 @@ export class SparseEdgeMap<T> extends AbstractEdgeMap<T> { | |||
|
|
|||
| @Override | |||
| containsKey(key: number): boolean { | |||
| return this.get(key) != null; | |||
| return !!this.get(key); | |||
There was a problem hiding this comment.
📝 The form seen here is idiomatic C++. Since JavaScript behaves the same way, would it be idiomatic JavaScript/TypeScript as well? Also, would !!this.get(key) or this.get(key) !== undefined be faster in practice?
There was a problem hiding this comment.
Yes, I think !! is widely accepted to convert a Booleanish to Boolean.
This pull request addresses cases where
undefinedandnullare being used inEdgeMap<T>without being part of type signatures.📝 Strict null checks are currently disabled, but we're working to change that. See #26.