4
\$\begingroup\$

What it is about

I created an application where you need to push invisible buttons in a certain order to unlock the advanced mode. To handle this, I wanted to use a queue (FIFO) of my own, adapted to my problem.

Every time I click a button, its ID is added to the queue with addItem, and the password gets checked with checkCorrect. It is meant to work similarly to digicode doors. The size attribute determines how many values the queue can remember, hence the maximum length of the password.

What I'd like to know

Do you think this is a good code? In which ways can it be improved? Are there other ways (with smaller / more elegant code) to implement this?

The code

class MyQueue():
    def __init__(self, size, default_value=None):
        self.size = size
        self.content = [default_value]*size
        self.end = self.size-1
    def addItem(self, value):
        self.end = (self.end+1) % self.size
        self.content[self.end] = value
    def printQueue(self):
        print 'end='+str(self.end)+' - '+str(self.content)
    def checkCorrect(self, password):
        pw = list(reversed(password))
        i = 0
        j = self.end
        if len(pw) > self.size:
            return False
        while True:
            if pw[i] <> self.content[j]:
                return False
            else:
                i += 1
                j = (j-1) % self.size
                if i == len(pw):
                    return True

if __name__=='__main__':
    q = MyQueue(size=4, default_value=0)
    q.addItem(1)
    q.addItem(8)
    q.addItem(8)
    q.addItem(5)
    q.addItem(7)
    q.addItem(3)
    print q.checkCorrect([5, 7, 3])
    print q.checkCorrect([2, 7, 3])
\$\endgroup\$
  • 1
    \$\begingroup\$ What is defaultValue ? You meant default_value I guess? More, do NOT use <> operator. It's kind of unusual and unsupported in Python 3.5 (even if you're using python 2.7). Use != instead. You're code as it is, it's off-topic. Please edit it. \$\endgroup\$ – Grajdeanu Alex. Jul 8 '16 at 12:08
  • \$\begingroup\$ I edited the variable name, thanks for noticing it. It didn't know for <> and !=, do you want to make it an answer ? Besides, please how can I make this less off-topic ? I don't understand how it is off-topic in the first place... \$\endgroup\$ – BusyAnt Jul 8 '16 at 12:09
  • \$\begingroup\$ Why are you implementing a queue yourself? docs.python.org/2/library/queue.html \$\endgroup\$ – Caridorc Jul 8 '16 at 13:07
  • \$\begingroup\$ Well I found this interesting, and I wanted some object that I fully understand, and adapted to my case. \$\endgroup\$ – BusyAnt Jul 8 '16 at 13:38
3
\$\begingroup\$

object

Since it is Python 2, you most likely want to use new-style classes instead of old ones. Meaning that MyQueue must explicitly inherit from object:

class MyQueue(object):
    ...

__iter__

Since you are creating a container, you should provide an easy way to iterate over its content. That easy way is done by implementing __iter__ which let you use:

container = MyQueue()
...  # Do stuff to populate container
for element in container:
    print(element)

A first approach could be:

class MyQueue(object):
    ...
    def __iter__(self):
        size = self.size  # cache value in local scope for faster access
        for i in xrange(self.end, self.end + size):
            yield self.content[i % size]

This helps you simplify checkCorrect:

    def checkCorrect(self, password):
        if len(password) != self.size:
            return False
        return all(val == passwd for val, passwd in zip(self, reversed(password)))
\$\endgroup\$
3
\$\begingroup\$

Some PEP8 guidance:

class MyQueue(): here, you can remove the redundant parentheses as you're not passing any parameter to it. So you will have class MyQueue.

When inside of a class, it's recommended to have one space between your methods:

def addItem:
    ....

def printQueue:
    ....

It's also specified in PEP8 the following (methods naming):

Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

That said, addItem would become add_item.

More

Don't use <> operator. It's not usual. I strongly recommend you using != operator (which is the common way of doing this) when you're trying to say not equal. Later on, you might want to use this with a more recent Python version (say Python 3.5). This way, you won't have any issues.

Related to portability, you might also want to add parentheses when using print(). It's not necessary since you're using Python 2.7, but it might help you in the future.

Classes and methods inside them usually have docstrings. To quote the documentation:

A docstring is a string literal that occurs as the first statement in a module, function, class, or method definition. Such a docstring becomes the doc special attribute of that object.

All modules should normally have docstrings, and all functions and classes exported by a module should also have docstrings.

That said:

class MyQueue:
    """Some text to describe what you're class does"""
    ....

    def add_item(self, value):
        """ what this method does """
        ...

In a perfect world, which obviously we're not living in, I would've expect to see some push / pop methods or queue / dequeue ones since we're talking about a queue implementation and it's way of handling things (FIFO). Perhaps add_item might be push_item.

More to come

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.