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.
