2
\$\begingroup\$

This code works, but my boss told me that it's very bad code, and don't want to deploy it. It is my first time working as a programmer and I have at least 3 months of experience.

# -*- coding: utf-8 -*-

import psycopg2.extras
import time
import json
from psycopg2.extensions import AsIs
from methods_grabber import get_district, get_city,\
                            get_street, get_house, get_postcode
time_start = time.time()

conn = psycopg2.connect("dbname=gym user=gyms")

def get_data():

    global conn

    dict_cur = conn.cursor(cursor_factory=psycopg2.extras.DictCursor)
    dict_cur.execute("""
                    SELECT *
                    FROM dblgis_grabber_firm;
                    """
                    )

    res = dict_cur.fetchall()
    dict_cur.close()
    return res

def update_data(id_row, row, data):
    global conn

    cur = conn.cursor()

    query = "UPDATE dblgis_grabber_firm SET %s=%s WHERE id=%s;"

    cur.execute(query, (AsIs(row), data, id_row))

    conn.commit()
    cur.close()
    return cur.rowcount

data = get_data()

def get_from_contacts(contact_group, row_name, representation, id_row):
    result = []
    for contacts in contact_group:
        for contact in contacts.get('contacts'):
            if contact.get('type') == representation:
                result.append(unicode(contact.get('text')))
    result = '; '.join(result)
    update_data(id_row, row_name, result)

def fill_data(instance):

    longitude = instance.get('longitude')
    latitude = instance.get('latitude')

    id_row = instance.get('id')

    contact_group = json.loads(instance.get('all_data', {})).get('contact_groups', {})


    if not instance.get('street'):
        street = get_street(longitude, latitude)
        if street:
            update_data(id_row, 'street', street)

    if not instance.get('street_number'):
        street_number = get_house(longitude, latitude)
        if street_number:
            update_data(id_row, 'street_number', street_number)

    if not instance.get('postcode'):
        postcode = get_postcode(longitude, latitude)
        if postcode:
            update_data(id_row, 'postcode', postcode)

    get_from_contacts(contact_group, 'phone_number', 'phone', id_row)
    get_from_contacts(contact_group, 'site', 'website', id_row)
    get_from_contacts(contact_group, 'email', 'email', id_row)


for d in data:
    fill_data(d)

conn.close()

print 'Timeit %s' % (time.time() - time_start)

It use my methods from methods, which resolves data from yandex and geopy lib.

After parsing big JSON data, it writes changes to the DB. It's tested for 4k elements and works for 230sec. I think it's normal, but why is it so bad?

\$\endgroup\$
3
  • \$\begingroup\$ Rewriting code to use Django ORM is outside the scope of Code Review. \$\endgroup\$ Feb 6 '16 at 5:51
  • 1
    \$\begingroup\$ I never ask to rewrite. I ask concrete - do you think it's a bad sample of code, even if it works? I mean, I know it's bad, it should be, but why it's a disaster, I can't get. \$\endgroup\$
    – kAldown
    Feb 6 '16 at 5:52
  • \$\begingroup\$ If every geocoding query requires an HTTP request, there probably isn't that much you can do to improve performance. \$\endgroup\$ Feb 6 '16 at 6:03
2
\$\begingroup\$

Avoid globals

There is no need in using the global keyword here since you’re not modifying your conn. But there is also no need to access this variable at global scope, you can easily pass it as parameter.

One of the advantages of using parameters instead of a global variable being that you can more easily test each function with a custom connection when you need to.

Avoid top-level code

conn = psycopg2.connect("dbname=gym user=gyms")
data = get_data(conn) # Adding parameter from previous advice
for d in data:
    fill_data(conn, d) # Adding parameter from previous advice
conn.close()

Is better within its own function: you get the workflow at first glance. You can also use the function parameters to pass the connection credentials.

Using a function here lets you easily write:

if __name__ == '__main__':
    time_start = time.time()
    check_data("dbname=gym user=gyms")
    print 'Timeit %s' % (time.time() - time_start)

You clearly see there what is computation and what is timming. This is also best practice to wrap the executable part of your code under if __name__ == '__main__'.

But you are usually better off using timeit for performances measurement.

Avoid managing database state yourself

psycopg2 provide context managers to wrap connection or cursor objects. For the former it handles auto-commit/rollback based on how things went within the context; and for the latter it auto-closes it.

You can thus write

def get_data(conn):
    with conn.cursor(cursor_factory=psycopg2.extras.DictCursor) as cursor:
        cursor.execute("""
                       SELECT *
                       FROM dblgis_grabber_firm;
                       """)
        return cursor.fetchall()

and for update_data, you’ll need to auto-commit/rollback the connection:

def update_data(conn, id_row, row, data):
    with conn:
        with conn.cursor() as cursor:
            cursor.execute(
                "UPDATE dblgis_grabber_firm SET %s=%s WHERE id=%s;",
                (AsIs(row), data, id_row)
            )

I removed the return statement since you never make use of it.

Improve get_from_contacts

First of, the name is misleading. It doesn't get anything, it set data in the database. It probably should be named update_from_contacts. Alternatively, you could keep the name get_from_contacts and have that function actually do what it claims to do; and nothing more:

def get_from_contacts(contact_group, representation):
    result = []
    for contacts in contact_group:
        for contact in contacts.get('contacts'):
            if contact.get('type') == representation:
                result.append(unicode(contact.get('text')))
    return '; '.join(result)

You will need to have update_data calls near your get_from_contacts; but it separate concerns. And you can save on number of parameters.

You could also improve the overall execution: initialyzing a list to the empty list and appending inside a for loop is often an anti-pattern in python and better served with a list comprehension. Here the join call calls to using a generator expression:

def get_from_contacts(contact_group, representation):
    return '; '.join(
        unicode(contact.get('text'))
        for contacts in contact_group
        for contact in contacts.get('contacts')
        if contact.get('type') == representation
    )

Avoid redundancy

In fill_data, there are several constructions that look alike. You should be able to extract the structure from the data and use iterations to achieve the desired result without copy/pasting similar blocks of code:

GEO_FETCH = {
    'street': get_street,
    'street_number': get_house,
    'postcode': get_postcode,
}

CONTACT_FIELDS = {
    'phone_number': 'phone',
    'site': 'website',
    'email': 'email',
}

def fill_data(conn, instance):
    longitude = instance.get('longitude')
    latitude = instance.get('latitude')

    id_row = instance.get('id')

    contact_group = json.loads(
        instance.get('all_data', {})
    ).get('contact_groups', {})

    for field, fetcher in GEO_FETCH.iteritems():
        if not instance.get(field):
            geo_data = fetcher(longitude, latitude)
            if geo_data:
                update_data(conn, id_row, field, geo_data)

    for row_name, field_name in CONTACT_FIELDS.iteritems():
        row_data = get_from_contacts(contact_group, field_name)
        update_data(conn, id_row, row_name, row_data)

Bringing it all together

# -*- coding: utf-8 -*-

import json
import psycopg2.extras
from psycopg2.extensions import AsIs
from methods_grabber import (get_district, get_city,
                             get_street, get_house, get_postcode)


GEO_FETCH = {
    'street': get_street,
    'street_number': get_house,
    'postcode': get_postcode,
}

CONTACT_FIELDS = {
    'phone_number': 'phone',
    'site': 'website',
    'email': 'email',
}

def fill_data(conn, instance):
    longitude = instance.get('longitude')
    latitude = instance.get('latitude')

    id_row = instance.get('id')

    contact_group = json.loads(
        instance.get('all_data', {})
    ).get('contact_groups', {})

    for field, fetcher in GEO_FETCH.iteritems():
        if not instance.get(field):
            geo_data = fetcher(longitude, latitude)
            if geo_data:
                update_data(conn, id_row, field, geo_data)

    for row_name, field_name in CONTACT_FIELDS.iteritems():
        row_data = get_from_contacts(contact_group, field_name)
        update_data(conn, id_row, row_name, row_data)

def get_from_contacts(contact_group, representation):
    return '; '.join(
        unicode(contact.get('text'))
        for contacts in contact_group
        for contact in contacts.get('contacts')
        if contact.get('type') == representation
    )

def update_data(conn, id_row, row, data):
    with conn:
        with conn.cursor() as cursor:
            cursor.execute(
                "UPDATE dblgis_grabber_firm SET %s=%s WHERE id=%s;",
                (AsIs(row), data, id_row)
            )

def get_data(conn):
    with conn.cursor(cursor_factory=psycopg2.extras.DictCursor) as cursor:
        cursor.execute("""
                       SELECT *
                       FROM dblgis_grabber_firm;
                       """)
        return cursor.fetchall()

def check_data(db_credentials):
    conn = psycopg2.connect(db_credentials)
    for data in get_data(conn):
        fill_data(conn, data)
    conn.close()


if __name__ == '__main__':
    import timeit
    timeit.timeit('check_data("dbname=gym user=gyms")',
                  number=1, globals={'check_data': check_data})

Beyond readable code

Good production code is not only clear but also correctly documented to be maintainable. You should spend some time writing some docstrings to help any future reader understand what your code does. Comments in the code can also help understand how the code accomplishes its goals.

\$\endgroup\$
2
  • \$\begingroup\$ Very appreciate, but is it better to open and close conn every sql execute? I make globals just for rely once to open and once to close. Thanks. \$\endgroup\$
    – kAldown
    Feb 7 '16 at 22:01
  • \$\begingroup\$ @kAldown I openned conn only once at the beginning of check_data and closed it only once at its end. The context manager associated to a connection object only ensures auto-commit/rollback; it neither closes nor creates the connection. See initd.org/psycopg/docs/usage.html#with-statement \$\endgroup\$ Feb 7 '16 at 22:05

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.