HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Have I coded this small object grouping script Pythonically?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisscriptgroupingpythonicallysmallcodedobjecthave

Problem

I have a small script that takes a list of objects from an SQL table and groups them if the date-time on the object has less than 60 second difference with the last object.

It does this by converting the date-time to epoch and then subtracting the epoch of the previous object from the epoch of the current object. The code works but I feel as if it is too convoluted for such a simple task.

rows = cur.fetchall() #gets all the objects from table

source = []
list1 = []
old_epoch = 1404413062 #I use the same epoch time as the first object but this isn't very pythonic

for a in rows:
    date_time = a[12] #gets date_time

#strips date_time with regex to match the proper pattern format
#sample date_time from db "2014-07-07 11:10:18.867024-04"
    date_time = re.match(r'^.*?\.', date_time).group(0) 
    date_time = date_time.replace(".", "")
    pattern = '%Y-%m-%d %H:%M:%S'

    #converts date_time to epoch
    epoch = int(time.mktime(time.strptime(date_time, pattern)))

    #had to add this small part to force the iterator to append the last chunk since no new epoch_time to compare and get difference as difference would be zero 
    if a == rows[-1]:
        source.append(a[11])
        list1.append(source)

    else:
        if epoch - old_epoch < 60:
            source.append(a[11])

        else:
            list1.append(source)
            source = []
            source.append(a[11])

    old_epoch = epoch


EDIT: Updated this post for those asking what it does. It returns a list of categorized groups based on the timestamp as displayed below when printing out list1. I replaced the actual objects with only their timestamp property for readability purposes. :

```
['2014-07-07 06:21:51.011377-04', '2014-07-07 06:21:51.347373-04', '2014-07-07 06:21:51.678615-04']
['2014-07-07 06:26:54.01491-04', '2014-07-07 06:26:54.347479-04', '2014-07-07 06:26:54.671736-04']
['2014-07-07 06:31:57.107409-04', '2014-07-07 06:31:57.437156-04', '2014-07-07 06:31:57.804104-04']

Solution

list1 is a bad variable name. What is it actually storing? I know that it is a list, but I don't know what it is being used for.

It looks like this code is a snippet from a bigger block of code, but at least in this context, list1 is never being read. You should make sure the value is used later or remove the variable.

a is also a bad name. A single letter variable name is rarely a good variable name.

Comments:

for a in rows:
    date_time = a[12] #gets date_time

#strips date_time with regex to match the proper pattern format
#sample date_time from db "2014-07-07 11:10:18.867024-04"
    date_time = re.match(r'^.*?\.', date_time).group(0)


Your comments should start at the same indentation level as the code. What you have now could make someone reading your code think there are two different blocks.

Most of your comments are saying what the code does. This is generally an indication that you need to make your code more descriptive. You can achieve this with better variable names and extracting chunks of code into well-named sub-functions. Comments are better suited at describing why something is being done.

if a == rows[-1]:
    source.append(a[11])
    list1.append(source)

else:
    if epoch - old_epoch < 60:
        source.append(a[11])

    else:
        list1.append(source)
        source = []
        source.append(a[11])


You don't need the extra line after the if/else blocks.

source = []
source.append(a[11])


is the same as

source = [a[11]]


pattern has a constant value, you can initialize it before the loop instead to resetting the value for every row.

if epoch - old_epoch < 60:


60 is a magic number. What does it mean? If it were to change, you would have to find all the places 60 appears and check if it means the same thing as this context. If it has a name, you can just change the value once.

Code Snippets

for a in rows:
    date_time = a[12] #gets date_time

#strips date_time with regex to match the proper pattern format
#sample date_time from db "2014-07-07 11:10:18.867024-04"
    date_time = re.match(r'^.*?\.', date_time).group(0)
if a == rows[-1]:
    source.append(a[11])
    list1.append(source)

else:
    if epoch - old_epoch < 60:
        source.append(a[11])

    else:
        list1.append(source)
        source = []
        source.append(a[11])
source = []
source.append(a[11])
source = [a[11]]
if epoch - old_epoch < 60:

Context

StackExchange Code Review Q#56452, answer score: 2

Revisions (0)

No revisions yet.