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

Changes in Queue. #150

Closed
HarsheetKakar opened this issue Mar 13, 2020 · 8 comments · Fixed by #152
Closed

Changes in Queue. #150

HarsheetKakar opened this issue Mar 13, 2020 · 8 comments · Fixed by #152

Comments

@HarsheetKakar
Copy link
Contributor

Description of the problem

Queue module needs some refactoring.

  • Queue methods are generally enqueue() and dequeue()
  • front and rear pointers need to be properties because of their frequent change.
  • type(x) is not self._dtype can be converted to _check_type(type(x),self._dtype)

This issue was briefly discussed in #139 (comment)

Further suggestions are welcome.

I will be taking on this issue, since I'm also working on LinkedListStack side by side.

@HarsheetKakar
Copy link
Contributor Author

@czgdp1807 kindly add the points I missed

@czgdp1807
Copy link
Member

  • Queue methods are generally enqueue() and dequeue()

Let's not go for it, push and popleft are in accordance with collections.deque.

  • front and rear pointers need to be properties because of their frequent change.

They both are currently attributes of the Queue class. What are the benefits of additionally defining them as properties?

@HarsheetKakar
Copy link
Contributor Author

if self.front is None:
            self.front = self.queue.head
        self.rear = self.queue.tail

it keeps referring back to self.queue it would be better if we can just move it in a different function/property. Less headache for the programmer and less things to change if we ever have to.

@HarsheetKakar
Copy link
Contributor Author

  • Queue methods are generally enqueue() and dequeue()

Let's not go for it, push and popleft are in accordance with collections.deque.

yeah now to think of it, deque is used as Queue in many places.

@HarsheetKakar
Copy link
Contributor Author

if self.is_empty:
            raise ValueError("Queue is empty.")

I also dont understand why value error is raised. Something like IndexError would be better (same is used in deque)

@czgdp1807
Copy link
Member

if self.front is None:
            self.front = self.queue.head
        self.rear = self.queue.tail

it keeps referring back to self.queue it would be better if we can just move it in a different function/property. Less headache for the programmer and less things to change if we ever have to.

Yes, this will help in removing redundant attributes.

if self.is_empty:
            raise ValueError("Queue is empty.")

I also dont understand why value error is raised. Something like IndexError would be better (same is used in deque)

Feel free to make the change. See, if such changes are needed elsewhere in the code base.
Thanks.

@HarsheetKakar
Copy link
Contributor Author

quick question...
elif type(items) in (list, tuple):
we are checking against list and tuple only, but I think items can be any collections.abc.Sequence type.
Shall I change it to elif _check_type(items, Sequence)

@czgdp1807
Copy link
Member

Shall I change it to elif _check_type(items, Sequence)

I won't recommend.

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

Successfully merging a pull request may close this issue.

2 participants