HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Python simple int subclass

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
subclasssimplepythonint

Problem

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 "", line 1, in 
  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')}

Solution

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" % 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


  • I explicitly call class method to get a priority from either the string or number. I dislike overloading the one constructor to take either.



  • For a small number of items, I prefer to create them all upfront. It makes the construction simpler



  • 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.

Code Snippets

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)
try:
   return Pri._dict[level] 
except KeyError:
   return super(Pri,cls).__new__(cls, level)

Context

StackExchange Code Review Q#6229, answer score: 3

Revisions (0)

No revisions yet.