1
\$\begingroup\$

I have the following code which removes a dict from a list of dicts in Python based on whether the Date key has a value greater than 180 days ago (ideally needs to be 6 months)

gp_clinicals_meds_repeat = session["gp_clinicals_meds_repeat"]
for i in range(len(gp_clinicals_meds_repeat["GpRepeatMedicationsList"])):
    date_object = parser.parse(gp_clinicals_meds_repeat["GpRepeatMedicationsList"][i]["Date"])
    months_between = datetime.now() - date_object
    if months_between.days > 180:
        del gp_clinicals_meds_repeat["GpRepeatMedicationsList"][i]

An example of my JSON is below (just has one entry but could have hundreds)

{
    "GpRepeatMedicationsList": [{
        "Constituent": "",
        "Date": "2021-07-15T00:00:00",
        "Dosage": "0.6ml To Be Taken Each Day",
        "LastIssuedDate": "2021-07-15T00:00:00",
        "MixtureId": "",
        "Quantity": "50",
        "ReadCode": "DADR8795BRIDL",
        "Rubric": "Dalivit oral drops (Dendron Brands Ltd)",
        "TenancyDescription": "Orglinks",
        "Units": "ml"
    }],
    "TotalItemCount": 1
}

My question is, how could I have wrote this better? The code works and is fine for my needs but was wondering if there was a better way to do it.

\$\endgroup\$
1
  • \$\begingroup\$ Does your session give you a JSON string? What is session? Can these data be loaded into Pandas? \$\endgroup\$
    – Reinderien
    Jun 22 at 13:01

3 Answers 3

3
\$\begingroup\$

Use a list comprehension! When constructing lists in python by looping through others lists, its a lot more visually de-cluttered (not necessarily more performant) to just use python's list comprehensions. You still have the single loop, but you don't need to do deletes on the previous list (which you might wish to store later for audit purposes or something).

# use timedelta to calculate the 180 day diff between dates
from datetime import datetime, timedelta

...

# This gives you that internal list of medications
gp_clinicals_meds_repeat = session["gp_clinicals_meds_repeat"]

cutoff_date = datetime.now() - timedelta(days=180)

# Should give you a list of json objects that respect the conditional at the end.
# If you are just running this as you consume an api and don't want to have pandas, it should work if your data isn't too ridiculously massive.
data_after_cutoff = [med_data for med_data in gp_clinicals_meds_repeat if med_data['Date'] > cutoff_date]
\$\endgroup\$
2
\$\begingroup\$

Bugs

The code has at least two bugs.

The first is when two adjacent list items are more than 6 months old, the second one will not be deleted. This occurs because when an item is deleted, the rest of the list items gets moved forward. When the item at index i is deleted, the items from index i+1 to the end get copied up one position. That is, the item at index i+1 gets copied to index i, etc. The for-loop then increments i and the item that was previously at index i+1 gets skipped.

The second bug is obvious. Any time a item is deleted from the list, an IndexError will be raised. When an item is deleted, the list is shortened. But the for-loop runs over the length of the original list, so i will run past the end of the shortened list.

Both bugs can be seen in this simple example:

test = [1,5,6,3,2]

for i in range(len(test)):
    print(i, test)
    if test[i]>3:
        del test[i]

Output:

0 [1, 5, 6, 3, 2]
1 [1, 5, 6, 3, 2]
2 [1, 6, 3, 2]
3 [1, 6, 3, 2]
4 [1, 6, 3, 2]
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)

The code deletes items > 3, but misses the '6' following the '5'. It also raised an IndexError.

One way to fix the code is to iterate over it in reverse:

test = [1,5,6,3,2]

for i in range(len(test)-1, -1, -1):
    print(i, test)
    if test[i]>3:
        del test[i]
        
print(test)

Outputs:

4 [1, 5, 6, 3, 2]
3 [1, 5, 6, 3, 2]
2 [1, 5, 6, 3, 2]
1 [1, 5, 3, 2]
0 [1, 3, 2]
[1, 3, 2]

This works because the portion of the list that gets moved because of a deletion have already been checked. And even if the list is shortened, it first index is always 0.

Performance

The current algorithm is O(n^2). The list comprehension proposed by am_an_attendant_lord is O(n), but uses additional memory. An O(n) in-place algorithm could look something like:

test = [1,5,6,3,2]

i = 0
for item in test:
    print(i, test)
    if item <= 3:
        test[i] = item
        i += 1
del test[i:]

print(test)

Compute things that don't change before the loop instead of in the loop. For example,

gp_repeat_medications_list = gp_clinicals_meds_repeat["GpRepeatMedicationsList"]` 

and

cutoff_date = (datetime.now() - timedelta(days=180)).date()

isoformat dates

The parts of an ISO formatted date string are in order of significance. The year comes first and the seconds come last. This is useful because the date strings can be compared directly with out converting the strings to datetime objects. In my testing, converting to datetime objects and comparing them takes 4 times longer than comparing the strings. (This presumes all the strings have the same time zone or all have no time zone).

Conclusion

Combine the above and get something like this

gp_clinicals_meds_repeat = session["gp_clinicals_meds_repeat"]
gp_repeat_medications_list = gp_clinicals_meds_repeat["GpRepeatMedicationsList"]

cutoff_date = (datetime.now() - timedelta(days=180)).date().isoformat()

i = 0
for item in gp_repeat_medications_list:
    
    if item["Date"] >= cutoff_date:
        gp_repeat_medications_list[i] = item
        i += 1
        
del gp_repeat_medications_list[i:]
\$\endgroup\$
-1
\$\begingroup\$

Depends on how much you care about performance. If you really care, don't use Python. If you kind of care, use Pandas in Python.

I cannot speak to how you load your data in Pandas because you haven't shown what session actually is/does. You may or may not be able to use Pandas' own JSON deserialisation. If you can't, just use from_records:

import pandas as pd

sample = {
    "GpRepeatMedicationsList": [{
        "Constituent": "",
        "Date": "2021-07-15T00:00:00",
        "Dosage": "0.6ml To Be Taken Each Day",
        "LastIssuedDate": "2021-07-15T00:00:00",
        "MixtureId": "",
        "Quantity": "50",
        "ReadCode": "DADR8795BRIDL",
        "Rubric": "Dalivit oral drops (Dendron Brands Ltd)",
        "TenancyDescription": "Orglinks",
        "Units": "ml"
    }]*200 + [{
        "Constituent": "",
        "Date": "2022-06-01T00:00:00",
        "Dosage": "0.6ml To Be Taken Each Day",
        "LastIssuedDate": "2021-07-15T00:00:00",
        "MixtureId": "",
        "Quantity": "50",
        "ReadCode": "DADR8795BRIDL",
        "Rubric": "Dalivit oral drops (Dendron Brands Ltd)",
        "TenancyDescription": "Orglinks",
        "Units": "ml"
    }],
    "TotalItemCount": 1
}


df = pd.DataFrame.from_records(sample['GpRepeatMedicationsList'])
df['Date'] = pd.to_datetime(df.Date)
expiry = pd.Timedelta(180, 'd')
age = pd.Timestamp.now() - df.Date
df = df[age < expiry]
\$\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.