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.

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

geo_edges: Use the more semantically meaningful Set #26

Closed
martinfrances107 opened this issue Nov 21, 2020 · 3 comments
Closed

geo_edges: Use the more semantically meaningful Set #26

martinfrances107 opened this issue Nov 21, 2020 · 3 comments

Comments

@martinfrances107
Copy link
Collaborator

martinfrances107 commented Nov 21, 2020

The last 2 commits have replaced the generic javascript object {} data objects with Map and Set data structures.

This is the last issue of that form.

I am about to attach a PR which show one of two possible approaches

The PR needs discussion as approach one has possible performance implications where as options two involves a backwards compatible breaking change.

So this set of judgement calls needs careful review.

In this PR to keep the return of geo_edges() to be an array I was forced chain and Array.from() into a .map()

return Array.from(_index).map(d => d.split("-").map(Number));
the JIT compiler may look at the chain of language primitives and do the correct thing - looping only once.

but on the face of it the PR introduces a "double looping structure."

My preferred solution is option two in which the return type of array becomes an iterator

... Obviously this would break all existing code bases in the wild.

making things faster is a concern #7

Maybe we should just leave geo_edges alone
Maybe the apparent double looping is acceptable.
Maybe we need to wait until the next major version number update?

@martinfrances107
Copy link
Collaborator Author

After a nights sleep ...

I think the ideal backward compatibility breaking solution would just simply return the Set. That would preserve the semantic meaning ...

That is

The function geo_edges() returns more than an array of edges .. it returns a Set of edges ( The elements are guaranteed to be be unique/ free of duplicates. )

@Fil
Copy link
Owner

Fil commented Nov 23, 2020

Alas… it wouldn't be so great, since Javascript sets do not work as expected with arrays, because two "identical" arrays are not equal: new Set([[0], [0]]).size === 2. This is why we do the awful "join to string / split string" thing. Because of this I think it's better to keep the result as an array. (I welcome your comments if you disagree :) )

@Fil Fil closed this as completed Nov 23, 2020
@martinfrances107
Copy link
Collaborator Author

Thank you for your reasoning ... I am sure now that you are correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants