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

Make change_priority API consistent #41

Merged
merged 4 commits into from
Nov 10, 2022
Merged

Conversation

liwenjieQu
Copy link

Currently for either PriorityQueue or DoublePriorityQueue, the return value for change_priority and change_priority_by functions are different.

In some cases where a customized (and complicated) struct P is used, change_priority_by is usually preferred which provides flexibility to update the underlying structure. However, due to nothing is returned, the caller has no idea whether the update is successful or not. There are few options

  1. make change_priority_by return an Option type (same as change_priority
  2. Call get_priority first. Based on result and then call change_priority_by
  3. Call push

2 and 3 are obviously not ideal, as Option2 caused IndexMap lookup twice; Option3 may cause a clone
This PR submits code change for Option1

@garro95
Copy link
Owner

garro95 commented Nov 9, 2022

I would rather return a bool then Option<()>.
Keep in mind that, anyway, the user could already use std::mem::replace, std::mem::swap or the unsafe take_mut crate to take out an Option(p1), where p1 is the old priority. For example, you could do something like

use priority_queue::PriorityQueue;
use std::mem::swap;

let mut pq = PriorityQueue::new();

pq.push("Apples", 5);
pq.push("Bananas", 8);
pq.push("Strawberries", 23);

let mut newp = Some(10);

pq.change_priority_by("Bananas", |p| {
  swap(p, newp.as_mut().unwrap());
});
let oldp = newp;
assert_eq!(pq.get("Bananas"), Some((&"Bananas", &10)));
assert_eq!(oldp, Some(8));

@liwenjieQu
Copy link
Author

Make sense to me. I will update the PR :-)

Either bool or Option will work for my case. The old value can be fetched in a way as you suggested.

@garro95
Copy link
Owner

garro95 commented Nov 9, 2022

Would you also be so kind to update the change_priority_by tests to check that the return value is always correct?

@garro95 garro95 merged commit 252550e into garro95:master Nov 10, 2022
@garro95
Copy link
Owner

garro95 commented Nov 10, 2022

Thanks for your contribution

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

Successfully merging this pull request may close these issues.

2 participants