Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm attempting to provide an agnostic means of identifying a member. I need a nickname, a guid, and one of user_id, email, or phone_number. Since I'm using Python, I try to not care about the Python type of member because, after all, if it has what I'm looking for then I don't really care whether it really is a Member... or so I have been taught.

There is a circumstance in my application in which actual Member instances may be lumped in with objects that represent the information necessary to create a member. Also, note that actual Member instances have a handy identification() method, which my code tries first.

The problem is in attempting to not care about its type. I've gone to the extent of trying attributes and items, and now my code looks incredibly repetitive.

try:
    return member.identification()
except AttributeError:
    pass

id_types = ['user_id', 'email', 'phone_number']
if isinstance(member, dict):
    if 'guid' not in member:
        member['guid'] = cls._next_guid()
    for idt in id_types:
        if idt in member:
            try:
                return {
                    'nickname': member['nickname'],
                    'guid': member['guid'],
                    idt: member[idt]
                }
            except KeyError:
                raise ValueError('no nickname')
    else:
        raise ValueError('no user_id, email, or phone_number')
else:
    if 'guid' not in member:
        member['guid'] = cls._next_guid()
    for idt in id_types:
        if hasattr(member, idt):
            try:
                return {
                    'nickname': member.nickname,
                    'guid': member.guid,
                    idt: getattr(member, idt)
                }
            except AttributeError:
                raise ValueError('no nickname')
raise ValueError('no identification could be made')

The question is, if my attempts to not care about the type to this extent are reasonable, then is there a way to make this code better adhere to the principle of DRY?

share|improve this question
    
Member instances may be lumped in with objects that represent the information necessary to create a member is suggest removing that situation first. Duck typing is about behavioral typing, it's not anything goes. If the caller doesn't pass in an object that is duck enough, that's the fault of the caller, don't try to outsmart it by adding unnecessary complexity. –  Lie Ryan Aug 22 at 4:55
    
Yeah that's the feeling I was getting as I was writing it, but @mleyfman has provided quite the neat solution for this specific case. –  rhgrant10 Aug 22 at 14:12

1 Answer 1

up vote 4 down vote accepted

I see 2 approaches to this:

  1. Allow Member to be initialized with a dictionary.
  2. Allow your Member class to behave like a dictionary.

If you proceed by approach 1, then your resulting code will be very concise:

if isinstance(member, Member):
    return member.identification()
else if isinstance(member, dict):
    member = Member(member)
    return member.identification()
raise ValueError('no identification could be made')

But you will have a lot of additional logic in Member

Alternatively, if you do not like that approach, we can make Member work like a dictionary. Basically member['guid'] would refer to member.guid if member is a Member or the dictionary version if member is a dictionary.

We can do this by adding some magic methods to Member: __getitem__(self, key): and __setitem__(self, key, val): and __contains__(self, val):. The code would look something like this:

def __getitem__(self, key):
    if key == "nickname":
        return self.nickname
    elif key == "guid":
        return self.guid
    # and so on

def __setitem(self, key, val):
    if key == "nickname":
        self.nickname = val
    elif key == "guid":
        self.guid = val
    # and so on

def __contains__(self, val):
    if val == "guid":
        return self.guid != None
    elif val == "user_id":
        return self.user_id != None
    # and so on

This would make code snippet something like this:

try:
    return member.identification()
except AttributeError:
    pass

if not isinstance(member, Member) and not isinstance(member, dict):
    raise ValueError('no identification could be made')

id_types = ['user_id', 'email', 'phone_number']
if 'guid' not in member:
    member['guid'] = cls._next_guid()
for idt in id_types:
    if idt in member:
        try:
            return {
                'nickname': member['nickname'],
                'guid': member['guid'],
                 idt: member[idt]
            }
        except KeyError:
            raise ValueError('no nickname')
 else:
    raise ValueError('no user_id, email, or phone_number')
share|improve this answer
    
Thanks! That's a pretty neat solution (and I should have come up with it myself, haha). A set of fresh eyes did the trick it would seem :) –  rhgrant10 Aug 22 at 14:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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