0
\$\begingroup\$

This is a pretty big undertaking for me with my basic skills, and I was kind of flying blind. It's basic in its intent: get a bunch of case info from a helpdesk app and put it into a database. There are >10k records, so I had to overcome some pagination challenges. It works, and I would love your opinions regarding best practices or making it more pythonic.

import time
import collections
from desk_auth import BASE_URL, service, session
import MySQLdb
import mysql.connector
from mysql.connector import errorcode
from db_info import uname,pw,host,db
import dateutil.parser


PAGER = 100

   def get_all_cases():
        """
    pages through items,
    returns a dict of dicts of case, cust, date
    #cases[case] = {cust:date}
    """
        i = 1
        all_cases = []
        is_next = ""

        while is_next is not None:
            endpoint_url = '/api/v2/cases/search'
            endpoint = ''.join([BASE_URL, endpoint_url])
            query_params = {
                'page':i,
                'per_page': PAGER,
                'status': 'new,open,pending', #,resolved,closed',
                'fields': 'created_at,status,labels,subject,updated_at'
                }
            response = session.get(endpoint, params=query_params)
            items = response.json()
            data = items['_embedded']['entries']
            is_next = items['_links']['next']
            for item in data:
                #case id, customer id
                case = get_clean_id(item['_links']['self']['href'])
                cust = get_clean_id(item['_links']['customer']['href'])

                #create date
                raw_date = item['created_at']
                p = dateutil.parser.parse(raw_date)
                date = '%s-%s-%s %s:%s:%s' % (p.year, p.month, p.day, p.hour, p.minute, '00')

                #last update
                raw_update = item['updated_at']
                l = dateutil.parser.parse(raw_update)
                last_update = '%s-%s-%s %s:%s:%s' % (l.year, l.month, l.day, l.hour, l.minute, '00')

                #subject, status
                subj = item['subject']
                status = item['status']

                #get plan from labels
                plan = get_plan(item['labels'])

                #get group id or set to 0
                if item['_links']['assigned_group'] is not None:
                    group = get_clean_id(item['_links']['assigned_group']['href'])
                else:
                    group = '0'

                #get user id or set to 0
                if item['_links']['assigned_user'] is not None:
                    assigned_to = get_clean_id(item['_links']['assigned_user']['href'])
                else:
                    assigned_to = 0

                cases = (case, cust, date, last_update, subj, status, plan, group, assigned_to)
                all_cases.append(cases)
            i += 1

            #prevents rate limiting over 60 calls/minute
            time.sleep(1)
        return all_cases

This inserts into the database:

    def add_cases(cases_dict):
        """
    Takes the list of known kanji and inserts into database.
    """
        cnx = mysql.connector.connect(user=uname, password=pw, host=host, database=db, charset='utf8')
        cursor = cnx.cursor()
        try:
            for data in cases_dict:
                print data
                sql = """INSERT INTO cases(id, customer_id, date, last_update, subject, status, plan, group_id, assigned_id)
    VALUES (%s, %s, %s , %s, %s, %s, %s, %s, %s);"""
                cursor.execute(sql, data)
                cnx.commit()

        except MySQLdb.Error, e:
            print "Error %d: %s" % (e.args[0], e.args[1])
            cnx.rollback()
        cursor.close()
        cnx.close()
\$\endgroup\$
1
\$\begingroup\$

Your code doesn't seem to be the complete module (e.g. get_clean_id() is referred to but not defined or imported, and I don't know what desk_auth.session does), so I won't try and understand the whole thing. I'll stick to comments about the general coding style.

Starting with your imports:

import time
import collections
from desk_auth import BASE_URL, service, session
import MySQLdb
import mysql.connector
from mysql.connector import errorcode
from db_info import uname,pw,host,db
import dateutil.parser

The first thing that comes to mind when I see this is that it doesn't look organised. Different people organise their imports differently, but to me it's a bad sign if there doesn't seem to be any order at all. The PEP-8 (coding standards for Python) suggest the following order:

  1. standard library imports
  2. related third party imports
  3. local application/library specific imports

I generally put a blank line between each of these sections.

Use judgement about when to use import ... and when to use from ... import .... I won't go into the full details here, but in short I avoid this when the imported names are very short or generic. Names like pw or db could easily get confused with variables elsewhere in your code.

   def get_all_cases():
        """
    pages through items,
    returns a dict of dicts of case, cust, date
    #cases[case] = {cust:date}
    """

You have an indentation issue here. It may just be an issue with the StackExchange markup, but if not you should fix it. The function should be declared at the beginning of the line (assuming it's not a class method for which you've cut out the definition of the class).

The name get_all_cases() is not very specific. Whether this is a problem will depend on how the function is going to be called. If it were a class method, then the context might be enough to make it meaningful (e.g. doctor.get_all_cases() is quite clear). In Python, free (non-method) functions are often called by reference to the module they live in, which can make it clear what it means in the same way as an object name would. However, you need to ask yourself whether this is clear enough. My instinct is that this function name needs to be clearer.

i = 1

Avoid single-character variable names for anything but the control variable on a short for loop (e.g. for i in range(10): print x[i]). For something that is used over a large function like this, give it a proper name. It looks like this should be called page.

Generally, I think the get_all_cases() function is too long. Breaking it into smaller functions will make the logic easier to follow.

If I'm reading this right, you have quite a simple flow:

have_next = True
while have_next:
    items, have_next = get_data()
    all_items += items

return items

(this is just a simple sketch; I'm not suggesting you use variable or function names like this).

Moving the API call into one function and the work to create the cases out of the raw API result into another function would make this structure more obvious.

            cases = (case, cust, date, last_update, subj, status, plan, group, assigned_to)
            all_cases.append(cases)

Using tuples to represent entities when you have more than two or three elements quickly gets unmaintainable. If you want to get the status out of this, you have to remember to do case[5], which isn't very clear to read. If you end up wanting to add another field to this, then you have to insert it at the end or risk breaking any piece of code that refers to the case elements.

One alternative to using a tuple is to declare a class type and then use instances of this class type to represent your cases, but this might be unnecessary overhead in this case (it depends how widely you're using these objects). A simpler alternative is to use a namedtuple. That way you can refer to case.subj, case.status etc. in your later code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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