Have I coded this small object grouping script Pythonically?

Question

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']
['2014-07-07 06:37:00.178693-04', '2014-07-07 06:37:00.509359-04', '2014-07-07 06:37:00.828529-04']
['2014-07-07 06:42:03.268083-04', '2014-07-07 06:42:03.594742-04', '2014-07-07 06:42:03.921035-04']
['2014-07-07 06:47:06.389844-04', '2014-07-07 06:47:06.717987-04', '2014-07-07 06:47:07.035479-04']
['2014-07-07 06:52:09.516735-04', '2014-07-07 06:52:09.846621-04', '2014-07-07 06:52:10.171093-04']
['2014-07-07 06:57:12.623216-04', '2014-07-07 06:57:12.952604-04', '2014-07-07 06:57:13.278989-04']

Answer 1

The answer by @unholysampler gave a lot of good points that will point you towards more Pythonic code. I want to speak about algorithm changes.

Your current algorithm is pretty nice. Its simple and easy to follow. However, even though its simple does not mean that it isn't some fluff that isn't needed. Below are a few points that will help remove some of that fluff:

1. Solutions that require comparison to the previous element are easily implemented using the zip function:

   # This will print '(4, 8), (8, 15), (15, 16), (16, 23), (23, 42)'
   my_numbers = [4, 8, 15, 16, 23, 42]
   for group in zip(my_numbers[:-1], my_numbers[1:]):
       print group
   

   Using this function saves you from having to use the old_epoch hard-coded value.

2. time.strptime creates a time object that holds all the parsed data. Instead of creating the object then immediately finding the seconds since the epoch, simply use the time object and its tm_sec property for comparison.

3. Instead of having to use a regex, you can use the rindex function and list splicing to remove the trailing -04 data from the database's time information. Using this along with the another strptime flag %f (which recognizes milliseconds) we can remove having to use regexes completely.

   NOTE: I'm making a (probably safe) assumption about the formatting of the database time info.

4. You only need one list to hold the groups of times. See my code below for its usage.

---

Here is my version of your code. Once the data has all been processed, each row of data will either be in a group with other consecutive objects or in a group by itself:

rows = cur.fetchall()
data_index = 11
time_index = 12

# Start with the first object in a group.
groups = [[row[0][data_index]]]
pattern = '%Y-%m-%d %H:%M:%S.%f'
time_diff = 60
for prev, next in zip(rows[:-1], rows[1:]):
    prev_time = time.strptime(prev[time_index][:prev.rindex('-')], pattern)
    next_time = time.strptime(next[time_index][:next.rindex('-')], pattern)

    # If the time was within 60sec, add it to the latest group.
    # Otherwise, create a new group.
    if next_time.tm_sec - prev_time.tm_sec < time_diff:
        groups[-1].append(next[data_index])
    else:
        groups.append([next[data_index]])

Answer 2

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.

Answer 3

General comment

It is hard to know what your script is trying to do (it doesn't return anything, it doesn't print anything). A few comments and simple test cases are always welcome.

Also, the way the code is presented does not make things easier to understand : awkward variable names, comments in weird places, unusual places for new lines, etc. I reckon you should have a look at PEP8. You'll find various tools to check your code and help your make it look better (pep8, pylint, pyflakes, pychecker).

Anyway, let's get started.

Date processing

The way you handle date is wrong on many different levels :

• You can probably ask your SQL request to give you the time so that you don't have any processing whatsoever.
• At the very least, you can try to ask it not to have anything after seconds so that you don't have to handle it yourself.
• Regexp is not the right tool to perform the kind of operation you are trying to do. Even if it was, you should try to do it in such a way that you don't need to perform ugly string processing after using the regexp. There is a popular quote by Jamie Zawinski: :

Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two problems.

• Your comments are not in the right places
• Reusing the same variable (with an ambiguous name) in different places makes things hard to understand as one cannot know what the content of the variable is supposed to be at a given time.

You could write this :

# format is "2014-07-07 11:10:18.867024-04"
complete_date = a[12]
# format is "2014-07-07 11:10:18."
truncated_date = re.match(r'^.*?\.', complete_date).group(0)
# format is "2014-07-07 11:10:18"
usable_date = truncated_date.replace(".", "")
epoch = int(time.mktime(time.strptime(usable_date, '%Y-%m-%d %H:%M:%S')))

Logic

This is the part I am more interested in. Starting from :

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])

I am not quite sure which variable(s) is (are) useful but let's try to improve this in such a way that code looks easier to understand but it really does the same thing.

First thing : the source.append(a[11]) can be factorised out of the two branches in the else block (you might want to use the pass keyword for this step). Once this is done, you have a test with an empty then block, it might be better to negate the condition.

We now have this :

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

Now, it doesn't seem obvious but the two statements in the outter then can be but in the other order. Then, we can again, move the common code out of the if structure. Then, we, have an block with a single if inside, it calls for an elif to be used here :

if a == rows[-1]:
    list1.append(source)
elif epoch - old_epoch >= 60:
    list1.append(source)
    source = []
source.append(a[11])

Doesn't this look much cuter ?