8

I've been looking around for the best way of doing this but I haven't really found anything completely convincing.

I am writing a system where you have User objects and a collection that manages those Users. Each User has a name and I want to specify a function in the manager that can either take the name of the User or the User object itself.

class UserManager: 
  def remove_user(self,user_or_username):
    #If user_or_username is a string
    remote.remove(user_or_username)
    #If user_or_username is a User object
    remote.remove(user_or_username.name)

Is there any nifty way of doing this or is usage of isinstance the way to go?

3
  • Yes, isinstanceshould be okay. Commented May 15, 2012 at 12:58
  • Polymorphism really only helps if you don't know at the point of call whether you have a name or a user object. Mostly that isn't the case. You should resist the temptation to overload functions unless you really need to. Here you have a method that when given a string just chains to another method. That's a code smell: you should just call the other method directly; when you have a username call manager.remove(username) directly. Commented May 15, 2012 at 13:07
  • Your point is valid @Duncan but in this case the manager is an intermediate layer and its job is to make the proper calls for the user of my application. I wanted to explore this option to know how much flexibility I had at my disposal! Commented May 15, 2012 at 14:21

4 Answers 4

5

A solution like mgilson's, but slightly different:

def remove_user(self,user_or_username):
    try:
        #If user_or_username is a User object
        username = user_or_username.name
    except AttributeError:   #Oops -- didn't works.  ask forgiveness ;-)
        #If user_or_username is a string
        username = user_or_username
    remote.remove(username)

Why? Because this way, AttributeErrors in remove() are not suppressed.

It might be irrelevant, but I prefer concentrating exception handling to those places I really inted to have them.

Sign up to request clarification or add additional context in comments.

4 Comments

For even better factoring, you might consider extracting a function for the try/except block. It might be useful elsewhere, too.
@KarlKnechtel Well, a username = getattr(user_or_username, 'name', user_or_username) would as well be a way to go...
and +1 for figuring out how to do this in a 1-liner (using getattr) -- Not that I generally perfer 1-liners ...
@glglgl Agreed -- Though I tend to think that LBYL vs. EAFP depends on the application (and programmer preference/background in other languages, etc.) ...
3

using isinstance is a good approach... There is one more approach for this solution

if hasattr(user_or_username, 'name'):
    # this object has <name> attribute
    remote.remove(user_or_username.name)
else:
    remote.remove(user_or_username)

Comments

2

Sometimes python people like to say "it's better to ask forgiveness than permission"...

  def remove_user(self,user_or_username):
    try:
        #If user_or_username is a User object
        remote.remove(user_or_username.name)
    except AttributeError:   #Oops -- didn't works.  ask forgiveness ;-)
        #If user_or_username is a string
        remote.remove(user_or_username)

But I say it's just a matter of preference really. You could also use isinstance just as easily if you know you're only going to be getting strings, or User instances.

Comments

2

I would use isinstance, but this also works:

def remove_user(self, user):
   if hasattr(user, "name"):
      self.remove(user.name)
   else:
      self.remove(user)

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.