1

Goal: Reduce the wordiness of a (Python) retrieve_method using IF/ELIF/ELSE conditionals.

Structure of the Object Table and Method: This particular retrieve_method calls the user object based on elements making up the User table in the database (e.g. username, firstname, lastname, email, etc.)

As of now, my code is working, however it could use some cleaner, neater code. I am uncertain if I should use keywords or *args. I'm still learning Python, so any informative suggestions would be greatly appreciated. (You will notice that I am using an abstract word something_unique at the moment to compare the input to the elements found in the table. If the two match, the method returns the matched item.)

How can I improve this? What is the best way to go? Pros? Cons?

Software: Python 2.7.9, SQLAlchemy 1.0.9


Code:

def retrieve_user(self, something_unique):
    if isinstance(something_unique, int):
        print 'retrieve_user_id: ', something_unique # added
        return self.session.query(User).\
            filter(User.id == something_unique).one()
    elif isinstance(something_unique, basestring):
        print 'retrieve_user: ', something_unique # added
        return self.session.query(User).\
            filter(func.lower(User.username) == func.lower(something_unique)).first() 
    elif isinstance(something_unique, basestring):
        print 'retrieve_user email', something_unique
        return self.session.query(User).\
            filter(func.lower(User.email) == func.lower(something_unique)).first()
    elif isinstance(something_unique, User):
        return something_unique
    else:
        raise ValueError('Value being passed is an object')
Steven Rumbalski
  • 44,786
  • 9
  • 89
  • 119
thesayhey
  • 938
  • 3
  • 17
  • 38

3 Answers3

1

One way would be to use keyword parameters:

def retrieve_user(self, id=None, name=None, email=None):
    if id:
        return self.session.query(User).\
            filter(User.id == id).one()
    if name:
        return self.session.query(User).\
            filter(func.lower(User.username) == func.lower(name)).first()
etc
georg
  • 211,518
  • 52
  • 313
  • 390
  • 2
    The problem here is that it appears that the function is being called without knowledge of the type of "something_unique" which is why he is calling isinstance. I may be mistaken though, in which case this is a good answer. – Alex Alifimoff Jan 26 '16 at 16:01
  • Yes, @Alex you are correct. The `something_unique` can be either `username` or `email` (basestring) or `id` (int) depending on the call which is why it is currently some abstract word and is checking for type. Maybe an assert statement should be added to this answer??? Secondly, I see you use `If` statements only. So, the next check (e.g. `email`) would be something like `if email: return....` – thesayhey Jan 26 '16 at 16:10
  • @georg. The code above becomes problematic when you look for a name. e.g. `api.retrieve_user('dance') line 85, in retrieve_user filter(User.id == id).one() File "/usr/local/lib/python2.7/site-packages/sqlalchemy/orm/query.py", line 2534, in one raise orm_exc.NoResultFound("No row was found for one()") NoResultFound: No row was found for one()` – thesayhey Jan 26 '16 at 17:41
  • 1
    @thesayhey: the function is supposed to be called like this `api.retrieve_user(name='dance')` – georg Jan 26 '16 at 18:01
  • I added assert statements as well to check for type: e.g. `assert isinstance(id, int)` – thesayhey Jan 26 '16 at 19:58
1

Without more details about when this is being called and in what context, I can only come up with a couple of suggestions to clean this up.

The suggestions are mainly to use local variables to store things you call often, i.e. self.session.query(User), as well as lowercasing the identifier to start off with.

I want to be clear, particularly with doing "identifier = func.lower(...)", you do end up in a situation where, if this ends up being an ID, you've done a bit of unnecessary work.

def retrieve_user(self, something_unique):
    query_service = self.session.query(User)
    identifier = func.lower(something_unique)
    if isinstance(something_unique, int):
        print 'retrieve_user_id: ', something_unique # added
        return query_service.filter(User.id == something_unique).one()
    elif isinstance(something_unique, basestring):
        print 'retrieve_user: ', something_unique # added
        return query_service.filter(func.lower(User.username) == identifier).first() 
    elif isinstance(something_unique, _basestring):
        print 'retrieve_user email', something_unique
        return query_service.filter(func.lower(User.email) == identifier).first()
    elif isinstance(something_unique, User):
        return something_unique
    else:
        raise ValueError('Value being passed is an object')

It's honestly not that unclean as a method. You might consider refactoring to a dictionary if you do this over and over again.

To do this, you would store a type as a key and a function as a value. You then could do...

def retrieve_user(self, something_unique, func_dict):
    for key in func_dict:
        if isinstance(something_unique, key): return func_dict[key](something_unique)
    raise ValueError("Value being passed is an object")

Please note that this last suggestion is semantic -- it doesn't really save you a ton of code, except in retrieve_user(...) itself, as you still need to define those functions for the dictionary object elsewhere! It certainly helps you decompose, and is definitely worth it if you use those functions in other places, or have a huge cascading series of elifs. Otherwise I would keep it as one function.

Alex Alifimoff
  • 1,850
  • 2
  • 17
  • 34
  • above you mentioned that the second block of the conditionals wouldn't execute, I see no difference here. – thesayhey Jan 26 '16 at 17:50
  • 1
    This is true. You need to change basestring so that it is an instance of your email object and not just the an instance of a string (or whatever it happens to be in this situation). I've added an underscore to clarify that it needs to be different from the first basestring. – Alex Alifimoff Jan 26 '16 at 18:04
  • Do you have any suggestions? OR would you suggest changing the code to check for a basestring and if so start a different check (conditional) based on query? Or is that too easily buggy? – thesayhey Jan 26 '16 at 18:13
  • 1
    I would probably move the 'elif' block that checks against the 'User' type up. Then your options depend on the architecture if your overall system. If you restrict usernames to not include the '@' character, you could easily check for that character to figure out if it is an email. If you have no restrictions on allowed username characters, you may need a more inventive solution. – Alex Alifimoff Jan 26 '16 at 18:19
  • Also remember to accept an answer once you've resolved the problem! :) – Alex Alifimoff Jan 26 '16 at 18:19
0

You can use a dictionary in python to get the behavior of a switch statement, and it will be more efficient than if/elif/else pattern. Good description with code here:

http://code.activestate.com/recipes/181064/

Example from the link:

#! /usr/local/bin/python

# /////////////////////////////////////////////////////////////////////////
# /**
# * Title: rochambeau.py
# *
# * Description: Rock, Scissors, Paper Game. 
# *              Shows a clean way of implementing a 'switch'
# *              statement in Python via a dictionary container. 
# *              The dictionary is made up of known 'named states' that
# *              are tested in sequence for their current 'state'.
# *
# * Copyright: Copyright (c) 2003
# *            This file is distributed as EXAMPLE SOURCE CODE ONLY!
# *            The following code is considered 'Freeware' and can be 
# *            freely copied/reused/distributed as needed. 
# *
# * Company: None
# * @author: Alan Haffner
# * @version 1.0
# */
# 
# /////////////////////////////////////////////////////////////////////////
# Date: 02/16/03


import os, sys
import string, random
import types

def cli():

   c = '?'
   while c not in 'rps':

      try:
         print
         # tailing index '[0]' picks only first char from input
         c = raw_input('\tPlease enter (r)ock, (p)aper or (s)cissors to play... ')[0]
      except IndexError:
         # bad input, so like get another...
         pass

      c = c.lower()

      #   x, q...   --> quit
      if c in ('x', 'q' ):
         raise 'USER_QUIT_ERROR'

   return c

if __name__=='__main__':

   errorCode = 0

   stateList = ['r', 'p', 's']

   validStates = { 'User Wins'      : (('p','r'), ('r','s'), ('s','p')),
                   'No One Wins'    : (('p','p'), ('r','r'), ('s','s')),
                   'Computer Wins'  : (('r','p'), ('s','r'), ('p','s')),
   }

   try:
      while 1:
         testTuple     = (None, None)
         userInput     =        None
         computerInput =       '?'

         userInput     = cli()
         computerInput = ( stateList[random.randint(0,2)] )

         testTuple = (userInput, computerInput)

         for select in validStates:
            if testTuple in validStates[select]:
               print
               print "You chose:         ", userInput
               print "The computer chose:", computerInput
               print " ****", select, " ****" 
               print

   # Note: By convention, all local exception 'constants' end 
   # in '_ERROR' regaurdless of their intended use. 
   except KeyboardInterrupt:
      print '\n' * 3
      print '[interrupted by user]'
      print '\n' * 3
   except 'USER_QUIT_ERROR':
      print '\n' * 3
      print '[interrupted by user]'
      print '\n' * 3
   except:
      # unexpected error
      print '\n' * 3
      traceback.print_exc()
      print '\n' * 3

      errorCode = 2

   sys.exit(errorCode)
Alex Alifimoff
  • 1,850
  • 2
  • 17
  • 34
Dzohs
  • 91
  • 4