Deque #144
Deque #144
Conversation
Braqzen
added
New Feature
There was a problem hiding this comment.
- We should test the code via in-language tests rather than Rust code.
- We do not need to import modules from the prelude e.g. the
assertis automatically imported for us - I'm uncertain about this implementation which involves 2 fields. It does not behave as I expect, but I could be incorrect in my assumption. I expect something to track where the start / end is via an index and then using the index to determine where we insert / remove the item based on the method called. I would reconsider this implementation
| } | ||
| } | ||
|
|
||
| /// Appends an element to the back of the deque. |
There was a problem hiding this comment.
| /// Appends an element to the back of the deque. | |
| /// Appends an element to the end of the deque. |
Sorry, I feel the need to be pedantic here.
There was a problem hiding this comment.
I guess it would be better to use conventional terminology (back, front etc), so that the users are comfortable with the library.
There was a problem hiding this comment.
I would say follow what Rust does here as usual.
There was a problem hiding this comment.
Is there a reason to not use a similar implementation from Rust @rostyslavtyshko ?
| self.back.push(value); | ||
| } | ||
|
|
||
| /// Inserts an element at the front of the deque. |
There was a problem hiding this comment.
| /// Inserts an element at the front of the deque. | |
| /// Inserts an element at the start of the deque. |
Here too.
There was a problem hiding this comment.
Same argument as here
| pub fn pop_front(ref mut self) -> Option<T> { | ||
| if self.front.is_empty() { | ||
| if !self.back.is_empty() { | ||
| self.front.push(self.back.remove(0)) |
There was a problem hiding this comment.
| self.front.push(self.back.remove(0)) | |
| self.back.remove(0) |
Is there a reason that we do not actually remove from the front? Here we remove the first item from the back but then we add it to the end of the front which effectively does not pop the item.
May need to run forc fmt here. and same below
There was a problem hiding this comment.
The idea is to unify the logic and always to pop from front, even if due to some reasons all the elements were in back
forc fmt is run by CI, so seems to be fine
|
|
Closing due to inactivity |
Type of change
DequelibraryNotes
Related Issues
Closes #134