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

Your implementatin needs decoupling #1

Closed
alibad opened this issue Sep 27, 2015 · 7 comments
Closed

Your implementatin needs decoupling #1

alibad opened this issue Sep 27, 2015 · 7 comments

Comments

@alibad
Copy link

alibad commented Sep 27, 2015

This implementation of the priority queue wouldn't work well with objects in external libraries. It will also stain business objects with data-structure-related methods and properties.

If I want to put an object in this queue, it has to inherit from PriorityQueueNode. This makes any objects I want to put in the queue very tightly coupled with the queue data structure. There is definitely a way to do this and get the same performance without that extreme coupling.

@BlueRaja
Copy link
Owner

There is definitely a way to do this and get the same performance without that extreme coupling.

No, there isn't. Have you read this?

That being said, I have created an implementation which is safer and easier to use (safety checks in the release version, thread-safe, enqueues any object), but haven't had time to test and release it yet. I will try to get around to that soon.

@alibad
Copy link
Author

alibad commented Sep 27, 2015

Ok, that adds some more context. But still...

Let me give you this very simple example. I have an int. Now I have to create an IntWrapper class, make it inherit from your class, take the cost of storing all the 3 extra properties (double, long, long), take in the extra cost of allocation, with the added disadvantage of complexity. Too much overhead for what someone would be looking for in a priority queue.

A priority queue is mainly used to enqueue and dequeue by priority (Min heap or max heap), not to check if the queue contains a certain value. You implementation is not improving performance of expected operations on a priority queue (in fact it's slightly adding a bit perf\memory overhead on those operations). If you want to optimize for lookup, just use a different data structure.

@BlueRaja
Copy link
Owner

A normal priority queue will have the exact same overhead, it will just be hidden from you (at the cost of a small amount of speed).

This priority queue was originally written to be as fast as humanly possible for path-finding applications specifically, so enqueuing primitives wasn't an initial design consideration. However, as I said above, I've now written a more widely-applicable implementation, I just need to find time to test and publish it.

@BlueRaja
Copy link
Owner

I've pushed what I have so far to the SafePriorityQueue branch. The 'fast' queue has been renamed to FastPriorityQueue, while the 'convenient' queue is SafePriorityQueue.

Please be aware that some behavior is liable to change (eg. I'm not sure if I want First to return null or throw an exception when the queue is empty; or if duplicate keys should be allowed in SafePriorityQueue since they cannot be allowed in FastPriorityQueue)

@BlueRaja
Copy link
Owner

I've completed the implementation and created a PR: #2. Would you like to test it out before I merge?

@BlueRaja
Copy link
Owner

BlueRaja commented Jan 4, 2016

PR has been merged! The new implementation is called SimplePriorityQueue. It doesn't have the coupling issues, it automatically resizes, it contains lots of safety checks, and it's thread-safe.

The original priority queue has been renamed to FastPriorityQueue.

@BlueRaja BlueRaja closed this as completed Jan 4, 2016
@alibad
Copy link
Author

alibad commented Jan 4, 2016

Hi Raja,

Sorry I haven't been able to respond and review. Glad to see the improvements!

Sent from my Phone

On Jan 4, 2016, at 3:01 AM, BlueRaja notifications@github.com wrote:

PR has been merged! The new implementation is called SimplePriorityQueue. It doesn't have the coupling issues, it automatically resizes, it contains lots of safety checks, and it's thread-safe.


Reply to this email directly or view it on GitHub.

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