0

I am trying to improve my program so that it conforms to good programming practices. So I am looking for suggestions on whether the way I have programmed something is a good way of doing it.

I have a module called dbfunctions.py in which I have defined:

dbparams = {
    'dbname': 'qualitysimparams',
    'tablename': 'qualityparams',
    'tablecols': ('numpeople', 'numreviews', 'prophunters', 
                  'utility_funcform', 'goods'
    )

and a function:

def obtainid_ifrecord(dbname, tablename, tablecols, values):
    '''Checks if there already exists a record with given <values>. 
    If so, returns the id of that record, otherwise returns zero.'''
    con, c = connecttodb()
    q1 = "use {0}".format(dbname)
    c.execute(q1)
    q2p1 = "select id from {0} ".format(tablename)
    q2p2 = "where " + " = %s and ".join(tablecols) + " = %s"
    q2 = q2p1 + q2p2    
    c.execute(q2, values)
    res = c.fetchall()
    c.close()
    con.close()
    if res:
        return res[-1][0]
    else:
        return 0

There are other functions and variables in addition to the above two, but they are not relevant for this post.

In another file I have a function:

def checkif_paramcomboexists(numpeople, numreviews, prophunters, 
                             utility_funcform, goods):
    '''Check in the database if the simulation has been run with the 
    specified parameters. If so return the id of that run.
    '''
    goodsjson = sjson.dumps(goods)

    # paramvalues: in same order as listed in dbf.dbparams['tablecols']
    paramvalues = (numpeople, numreviews, prophunters, 
                   utility_funcform, goodsjson)

    id = dbf.obtainid_ifrecord(dbf.dbparams['dbname'],
                               dbf.dbparams['tablename'], 
                               dbf.dbparams['tablecols'],
                               paramvalues)
    return id

It seems to me that the fact that hardcoding the variable names in the paramvalues variable in function checkif_paramcomboexists is not a good practice. If later I change the order of variables in dbfunctions.dbparams['tablecols'] for any reason, checkif_paramcomboexists function will fail (and can fail silently depending on the data types). One way to get around this is to define:

paramvalues = [eval(x) for x in dbf.dbparams['tablecols']]

But I have heard that generally it is a bad practice to use eval (although I do not know why and when it is okay to use it). My questions are:

(i) Is it okay the way I have coded this in regards to the concern I have? I think the answer is 'No', but just want to check with the experts here. (ii) Is use of eval as I have indicated an acceptable solution? (iii) If answer to (ii) is 'no', what is the alternative?

Thank you for reading through this.

Curious2learn
  • 31,692
  • 43
  • 108
  • 125
  • 1
    This really belongs on [code review](http://codereview.stackexchange.com/). – Gareth Latty Dec 19 '12 at 14:40
  • Also, you shouldn't try to build your own query strings. All the python DB driver libraries provide mechanisms for composing queries with parameterized values, and they will handle most of the tricky edgecases of type conversion, quoting, escaping, and the like for you. – Silas Ray Dec 19 '12 at 14:45
  • Thanks sr2222. Are you referring to the SQLAlchemy, web2py DAL etc.? – Curious2learn Dec 19 '12 at 14:52
  • No, those are ORMs, I'm talking about the lower level drivers that those run on top of; `MySQLdb`, `cx_Oracle`, etc. – Silas Ray Dec 19 '12 at 14:55
  • This is hardly off-topic. It can be answered and is about good programming practice. Any specific reasons why you closed this other than showing off that you can close questions by deeming them as off-topic. – Curious2learn Dec 19 '12 at 21:35

2 Answers2

3

This situation really calls for an object. You are duplicating what is essentially instance information about a specific database table in 2 places, so it would make sense to make both these functions in to methods of some sort of database table interface object that has a tablecols attribute, then use self.tablecols in both methods.

Silas Ray
  • 25,682
  • 5
  • 48
  • 63
  • This is not clear to me. In one case the column names are listed as a tuple of strings. In the other case it is a tuple of values (which can be int, float, string etc.). What will the `tablecols` attribute look like? Won't I have to use `eval` if that attribute is a tuple of string. Thanks! – Curious2learn Dec 19 '12 at 14:55
  • You're right, the names are completely irrelevant from a code perspective in the second function. The only thing that really matters is which index in that list is goods, since you have to transform that in to json. You could then do something like `args = args[:self.dbparams.index('goods')] + [sjson.dumps(args[self.dbparams.index('goods')])] + args[self.dbparams.index('goods') + 1:]` – Silas Ray Dec 19 '12 at 15:42
3

You're right about the hardcoding not being great, and definitely stay away from eval. If you don't want to use *args or **kwargs (which are really better options, by the way), you can use the inspect module to do what you're trying to do.

import inspect, collections
def checkif_paramcomboexists(numpeople, numreviews, prophunters, 
                             utility_funcform, goods):

    ...

    temp = inspect.getargvalues(inspect.currentframe())
    args = temp[0]
    valuedict = temp[-1]
    ordered_args_dict = collections.OrderedDict(sorted(valuedict.items(), key=lambda x: args.index(x[0])))
    paramvalues = ordered_args_dict.values()

    ...

Basically, what's going on here is that inspect.getargvalues(inspect.currentframe()) gets you an object where the first item is a properly ordered list of the argument names and the last item is a dictionary of the argument names and values. We then create an ordered dictionary by grabbing the argument name/value mapping from the dictionary, and ordering it based on the list order.

What you end up with is an OrderedDict that has all of the arguments with their values, but also has them in the right order. That way, you can still choose to refer to them by name (e.g., ordered_args_dict['numpeople']), but if you can still get all the values in order as you wanted with ordered_args_dict.values(), which will give you the output you're looking for for paramvalues: a properly ordered list of the arguments, no matter what the name is.

Community
  • 1
  • 1
jdotjdot
  • 16,134
  • 13
  • 66
  • 118
  • Why go to the trouble of using `inspect` when you can much more easily just use `*args` and/or `*kwargs` mapped over `dbparams`? Besides, this still means that any changes to the order of the params requires changing the function signature. – Silas Ray Dec 19 '12 at 15:45
  • I completely agree, which is why I suggested `*args` or `**kwargs`. But especially given we don't have _all_ of his code, it may be more practical for him to finish this this way rather than rewriting all his code to deal with `args` or `kwargs` if in some places it expects keywords and other places it expects simply an ordered iterable for `paramvalues`. – jdotjdot Dec 19 '12 at 15:50
  • @jdotjdot thanks. I had not thought about *args and **kwargs. That may work great. I am trying to think how I would use it. If you can give an example with the function above, that would be great. – Curious2learn Dec 19 '12 at 16:10
  • I could see that. And actually, using the `inspect` pattern could be a benefit, as you can then keep the order of arguments, and thus the function signature the same while mapping the argument order to some single changeable argument index within the function when translating to and from the DB. – Silas Ray Dec 19 '12 at 16:23
  • @Curious2learn better off asking that in a new SO question – jdotjdot Dec 21 '12 at 02:04