Skip to content
This repository has been archived by the owner on Dec 2, 2021. It is now read-only.

Removed 'compare' functions and other unnecessary bits #10

Merged
merged 1 commit into from
Feb 22, 2019
Merged

Removed 'compare' functions and other unnecessary bits #10

merged 1 commit into from
Feb 22, 2019

Conversation

joshuahhh
Copy link
Contributor

@joshuahhh joshuahhh commented Sep 17, 2018

Per #9. Thanks!

@IjzerenHein
Copy link
Owner

Awesome, I will review and test this shortly!
Thanks for the contribution!

* @param {Number} c weak
* @param {Number} [w] weight
* @return {Number} strength
* @param a strong
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why remove the jsdoc type annotations?

Copy link
Contributor Author

@joshuahhh joshuahhh Sep 25, 2018

Choose a reason for hiding this comment

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

Reason is that the types are already present as TypeScript argument annotations. But I don't feel strongly about this at all. What happened is that my lint tool called this out as an unnecessary redundancy, and it sounded like a reasonable change to me.

(FWIW, use of jsdoc is pretty inconsistent across this codebase. I would be happy to follow whatever guidelines people want.)

@adamhaile
Copy link
Contributor

Hey, I'm the guy who pushed #8 and made that code obsolete. Yeah, this totally makes sense. My initial push intentionally didn't change any code beyond maptype.ts -- I was changing enough in @IjzerenHein 's code already so preserved the code that called into createMap(). Since that change has been accepted, this cleanup makes perfect sense. 👍

The old map used an associative array internally, and the supplied compare function was used to sort items such that lookups were O(log N). The new one uses a non-sorted array and an index map for O(1) lookups. compare therefore isn't needed.

@joshuahhh
Copy link
Contributor Author

Thanks for taking a look, @adamhaile! (And for #8 too.)

@joshuahhh
Copy link
Contributor Author

How do you think this looks, @IjzerenHein?

@IjzerenHein IjzerenHein merged commit de28399 into IjzerenHein:master Feb 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants