Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix handling of null and undefined in EdgeMap #65

Merged
merged 1 commit into from
Oct 10, 2016
Merged

Fix handling of null and undefined in EdgeMap #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/dfa/AbstractEdgeMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,8 @@ export abstract class AbstractEdgeMap<T> implements EdgeMap<T> {
// @Override
abstract containsKey(key: number): boolean;

// @Nullable
// @Override
abstract get(key: number): T;
abstract get(key: number): T | undefined;

// @NotNull
// @Override
Expand Down
26 changes: 13 additions & 13 deletions src/dfa/ArrayEdgeMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import * as assert from 'assert';
* @author Sam Harwell
*/
export class ArrayEdgeMap<T> extends AbstractEdgeMap<T> {
private arrayData: T[];
private arrayData: (T | undefined)[];
private _size: number;

constructor(minIndex: number, maxIndex: number) {
Expand All @@ -62,22 +62,22 @@ export class ArrayEdgeMap<T> extends AbstractEdgeMap<T> {

@Override
containsKey(key: number): boolean {
return this.get(key) != null;
return !!this.get(key);
}

@Override
get(key: number): T {
get(key: number): T | undefined {
if (key < this.minIndex || key > this.maxIndex) {
return null;
return undefined;
}

return this.arrayData[key - this.minIndex];
}

@Override
put(key: number, value: T): ArrayEdgeMap<T> {
put(key: number, value: T | undefined): ArrayEdgeMap<T> {
if (key >= this.minIndex && key <= this.maxIndex) {
let existing: T = this.arrayData[key - this.minIndex];
let existing: T | undefined = this.arrayData[key - this.minIndex];
this.arrayData[key - this.minIndex] = value;
if (existing == null && value != null) {
this._size++;
Expand All @@ -91,7 +91,7 @@ export class ArrayEdgeMap<T> extends AbstractEdgeMap<T> {

@Override
remove(key: number): ArrayEdgeMap<T> {
return this.put(key, null);
return this.put(key, undefined);
}

@Override
Expand Down Expand Up @@ -142,7 +142,7 @@ export class ArrayEdgeMap<T> extends AbstractEdgeMap<T> {

let result: Map<number, T> = new Map<number, T>();
for (let i = 0; i < this.arrayData.length; i++) {
let element: T = this.arrayData[i];
let element: T | undefined = this.arrayData[i];
if (element == null) {
continue;
}
Expand All @@ -159,10 +159,10 @@ export class ArrayEdgeMap<T> extends AbstractEdgeMap<T> {
}

class EntrySet<T> implements Iterable<{ key: number, value: T }> {
private readonly arrayData: T[];
private readonly arrayData: (T | undefined)[];
private readonly minIndex: number;

constructor(arrayData: T[], minIndex: number) {
constructor(arrayData: (T | undefined)[], minIndex: number) {
this.arrayData = arrayData;
this.minIndex = minIndex;
}
Expand All @@ -173,11 +173,11 @@ class EntrySet<T> implements Iterable<{ key: number, value: T }> {
}

class EntrySetIterator<T> implements Iterator<{ key: number, value: T }> {
private readonly arrayData: T[];
private readonly arrayData: (T | undefined)[];
private readonly minIndex: number;
private index: number;

constructor(arrayData: T[], minIndex: number) {
constructor(arrayData: (T | undefined)[], minIndex: number) {
this.arrayData = arrayData;
this.minIndex = minIndex;
this.index = 0;
Expand All @@ -193,7 +193,7 @@ class EntrySetIterator<T> implements Iterator<{ key: number, value: T }> {
let reachedEnd = this.index >= this.arrayData.length;
let result = { done: reachedEnd, value: reachedEnd ? undefined : { key: this.index + this.minIndex, value: this.arrayData[this.index] } };
this.index++;
return result;
return result as any as IteratorResult<{ key: number, value: T }>;
}

return?(value?: any): IteratorResult<{ key: number, value: T }> {
Expand Down
5 changes: 2 additions & 3 deletions src/dfa/EdgeMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,10 @@ export interface EdgeMap<T> {

containsKey(key: number): boolean;

// @Nullable
get(key: number): T;
get(key: number): T | undefined;

// @NotNull
put(key: number, /*@Nullable*/ value: T): EdgeMap<T>;
put(key: number, value: T): EdgeMap<T>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.


// @NotNull
remove(key: number): EdgeMap<T>;
Expand Down
4 changes: 2 additions & 2 deletions src/dfa/EmptyEdgeMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ export class EmptyEdgeMap<T> extends AbstractEdgeMap<T> {
}

@Override
get(key: number): T {
return null;
get(key: number): undefined {
return undefined;
}

@Override
Expand Down
17 changes: 9 additions & 8 deletions src/dfa/SingletonEdgeMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import { SparseEdgeMap } from './SparseEdgeMap';
export class SingletonEdgeMap<T> extends AbstractEdgeMap<T> {

private key: number;
private value: T;
private value: T | undefined;

constructor(minIndex: number, maxIndex: number, key: number, value: T) {
super(minIndex, maxIndex);
Expand All @@ -49,15 +49,15 @@ export class SingletonEdgeMap<T> extends AbstractEdgeMap<T> {
this.value = value;
} else {
this.key = 0;
this.value = null;
this.value = undefined;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 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?

}
}

getKey(): number {
return this.key;
}

getValue(): T {
getValue(): T | undefined {
return this.value;
}

Expand All @@ -77,12 +77,12 @@ export class SingletonEdgeMap<T> extends AbstractEdgeMap<T> {
}

@Override
get(key: number): T {
get(key: number): T | undefined {
if (key === this.key) {
return this.value;
}

return null;
return undefined;
}

@Override
Expand Down Expand Up @@ -132,10 +132,11 @@ export class SingletonEdgeMap<T> extends AbstractEdgeMap<T> {

@Override
entrySet(): Iterable<{ key: number, value: T }> {
if (this.isEmpty()) {
let value: T | undefined = this.value;
if (!value) {
return [];
} else {
return [{ key: this.key, value: this.value }];
}

return [{ key: this.key, value: value }];
}
}
8 changes: 4 additions & 4 deletions src/dfa/SparseEdgeMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think !! is widely accepted to convert a Booleanish to Boolean.

}

@Override
get(key: number): T {
get(key: number): T | undefined {
// Special property of this collection: values are only even added to
// the end, else a new object is returned from put(). Therefore no lock
// is required in this method.
let index: number = Arrays.binarySearch(this.keys, 0, this.size(), key);
if (index < 0) {
return null;
return undefined;
}

return this.values[index];
Expand Down Expand Up @@ -221,7 +221,7 @@ class EntrySetIterator<T> implements Iterator<{ key: number, value: T }> {

next(value?: any): IteratorResult<{ key: number, value: T }> {
if (this.index >= this.values.length) {
return { done: true, value: undefined };
return { done: true, value: undefined } as any as IteratorResult<{ key: number, value: T }> ;
}

let currentIndex = this.index++;
Expand Down