3
\$\begingroup\$

The below is a class designed to implement Rememember the Milk's task priorities. I'm using it partly as an exercise in "Pythonic" programming, so it's fairly simple already, but advice on how to make it simpler or more Pythonic would be particularly appreciated.

I'm using Python 2.6.5 in case it makes any difference.

class Pri(int):    # Subclass int to get handy comparison functions etc.
    '''Task priority'''

    public_from_internal = {1: 1, 2: 2, 3: 3, 4: 'N'}
    internal_from_public_str = {'N': 4, '1': 1, '2': 2, '3': 3}
    _dict = dict()

    def __new__(cls, level):
        try:
            level = Pri.internal_from_public_str[str(level)]
        except KeyError:
            raise ValueError, "Pri must be 'N', '1', '2', or '3'"

        # If there's already an instance of this priority, don't create a new one.
        if level in Pri._dict: return Pri._dict[level]
        else: return super(Pri, cls).__new__(cls, level)

    def __init__(self, level):
        super(Pri, self).__init__()
        Pri._dict[self] = self

    def __repr__(self): return "Pri('" + str(Pri.public_from_internal[self]) + "')"

    # Priority 1 is clearly greater than priority 3, so invert cmp
    def __cmp__(self, other): return (-cmp(int(self), int(other)))

Some selected output, for the curious:

>>> Pri(1)
Pri('1')
>>> Pri('N')
Pri('N')
>>> Pri(4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "rtm.py", line 10, in __new__
    raise ValueError, "Pri must be 'N', '1', '2', or '3'"
ValueError: Pri must be 'N', '1', '2', or '3'
>>> Pri._dict
{Pri('1'): Pri('1'), Pri('N'): Pri('N')}
>>> Pri(1) > Pri(2)
True
>>> bool(Pri('N'))
True
>>> repr(Pri("2"))
"Pri('2')"
>>> print repr(Pri('N'))
Pri('N')
>>> Pri(1)._dict
{Pri('1'): Pri('1'), Pri('2'): Pri('2'), Pri('N'): Pri('N')}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I'm curious as to why you're extending int -- I don't see any benefit from it here. Maybe I missed something? \$\endgroup\$ Nov 23, 2011 at 19:22
  • \$\begingroup\$ @Matt: No. It made sense in my mind at the time, but looking again it's entirely redundant now. \$\endgroup\$
    – me_and
    Nov 24, 2011 at 10:31

1 Answer 1

3
\$\begingroup\$
class Pri(int):    # Subclass int to get handy comparison functions etc.

Abbreviating words in class names makes it harder to follow. Just call it Priority

    '''Task priority'''

    public_from_internal = {1: 1, 2: 2, 3: 3, 4: 'N'}
    internal_from_public_str = {'N': 4, '1': 1, '2': 2, '3': 3}
    _dict = dict()

Naming this _dict doesn't give me a hint about what its used for

    def __new__(cls, level):
        try:
            level = Pri.internal_from_public_str[str(level)]

I don't like stringifying anything that gets passed in here. You really only want to support strings and ints not things that happen to stringify into the correct types.

        except KeyError:
            raise ValueError, "Pri must be 'N', '1', '2', or '3'"

        # If there's already an instance of this priority, don't create a new one.
        if level in Pri._dict: return Pri._dict[level]
        else: return super(Pri, cls).__new__(cls, level)

Use the following construct instead:

try:
   return Pri._dict[level] 
except KeyError:
   return super(Pri,cls).__new__(cls, level)

I think its clearer, and it will be slightly faster.

    def __init__(self, level):
        super(Pri, self).__init__()
        Pri._dict[self] = self

I do this is __new__ and not have an __init__ function.

    def __repr__(self): return "Pri('" + str(Pri.public_from_internal[self]) + "')"

I don't like putting the method name and implementation on the same line. I think it makes it look clutered. I'd also recommend using return "Pri(%s)" % Pri.public_from_internal[self] as I think its clearer as to the result.

    # Priority 1 is clearly greater than priority 3, so invert cmp
    def __cmp__(self, other): return (-cmp(int(self), int(other)))

You inherited from int to get comparisons. But then you have to reimplement the comparisons anyways. Now you also have all the other stuff like adding/multipyling/subtracting/etc which don't make sense to apply to priorities. Basically, there is no reason to inherit from int here, and many reasons not to.

How I'd do this

class Priority(object):
    priorities_by_name = {}
    @classmethod
    def from_string(cls, name):
         return cls.priorities_by_name[name]

    priorities_by_number = {}
    @classmethod
    def from_number(cls, number):
         return cls.priorities_by_number[number]

    def __init__(self, name, number):
        self.priorities_by_name[name] = self
        self.priorities_by_number[number] = self
        self.name = name
        self.number = number

    def __repr__(self):
        return "Priority<%d>" % self.number

    def __cmp__(self, other):
        return -cmp(self.number, other.number)


Priority.First = Priority('1', 1)
Priority.Second = Priority('2', 2)
Priority.Third = Priority('3', 3)
Priority.None = Priority('N', 4)

Priority.__init__ = None # Prevent any future creations
  1. I explicitly call class method to get a priority from either the string or number. I dislike overloading the one constructor to take either.
  2. For a small number of items, I prefer to create them all upfront. It makes the construction simpler
  3. I can access Priority objects at Priority.First (etc)

Truth be told I probably wouldn't implement a Priority class, instead I'd keep track of the priority as an int.

\$\endgroup\$
8
  • \$\begingroup\$ Truth be told, I probably wouldn't implement a Priority class if this were for "normal" development. As I mentioned in the question, I'm writing it partly as an exercise in using some of the bits of Python's class model that I'm less familiar with. \$\endgroup\$
    – me_and
    Nov 24, 2011 at 9:22
  • \$\begingroup\$ Why're you subclassing object here? I'm not sure what benefit that provides… \$\endgroup\$
    – me_and
    Nov 24, 2011 at 10:32
  • \$\begingroup\$ Also, why that definition of __repr__? The docs suggest repr should result in a string that will return the same value if pushed through eval, or something of the form <...> otherwise, and that seems to match neither. \$\endgroup\$
    – me_and
    Nov 24, 2011 at 10:42
  • \$\begingroup\$ Ack. I'm worried those questions sound like I'm being overly defensive. I'm very grateful for the comments, thank you! The suggestion to use @classmethod in particular is useful; I hadn't really understood what that did without those examples. \$\endgroup\$
    – me_and
    Nov 24, 2011 at 10:43
  • \$\begingroup\$ Ah. A little Googling answered my own question about subclassing object: it's a new-style/old-style class thing. For future readers: if you don't subclass anything or only subclass old-style classes, you get an old-style class. If you subclass a new-style class, where object is the simplest such, you get a new-style class. More info at the Python Wiki. \$\endgroup\$
    – me_and
    Nov 24, 2011 at 12:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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