# I am trying to filter a queryset based on dates, I keep getting a duplicate error on the for loops,How do I refactor these code to remove duplicates? [closed]

I am trying to filter the events table to get ongoing events and complete events, but pycharm keeps underlining my code because of duplicated code. How do I prevent for loop code duplicates on the two functions below? Better yet, how do I optimally refactor these two functions? Thanks


def get_ongoing_events():
ongoing_events = Events.objects.filter(
Q(event_begin_datetime__lte=current_time),
Q(event_end_datetime__gt=current_time),
)
for event in ongoing_events:
event.event_status = 'ongoing'
event.save()

event.venue.status = 'booked'
event.venue.save()

reserve_data = dict()
reserve_data["sensor_id"] = event.venue.sensor_id
reserve_data["status"] = event.venue.status

return reserve_data

def get_complete_reservation():
"""
Update reservations and sensors
:return:
"""
completed_events = Events.objects.filter(
Q(reservation_begin_datetime__lt=current_time),
Q(reservation_end_datetime__lte=current_time),
)

for event in completed_events:
event.reservation_status ='complete'
event.save()

event.venue.status = 'free'
event.venue.save()
reserve_data = dict()
reserve_data["sensor_id"] = event.venue.sensor_id
reserve_data["status"] = event.venue.status
return reserve_data
$$$$

• The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. Apr 26, 2021 at 7:30
• Apr 26, 2021 at 7:30

# Factoring duplication out

Whenever I have (near-)duplicate code, what I do is take a look at the repeated code. The parts that change ever so slightly are going to be controlled with arguments to functions and etc, whereas the parts that remain the same will just be left more-or-less as-is.

Having said that, here is what I see:

• the queries being used change, so those are function arguments;
• the attributes of the event being updated change, so those are function arguments;
• the final dict being generated is the same, so we leave it the same.

From your code alone, here is a suggested modification:

def filter_and_update_events(Qs, event_changes, venue_changes):
for event in Events.objects.filter(*Qs)
for attr, new_value in event_changes.items():
setattr(event, attr, new_value)
event.save()
for attr, new_value in venue_changes.items():
setattr(event.venue, attr, new_value)
event.venue.save()

reserve_data = dict()
reserve_data["sensor_id"] = event.venue.sensor_id
reserve_data["status"] = event.venue.status
return reserve_data

def get_ongoing_events():
return filter_and_update_events(
[
Q(event_begin_datetime__lte=current_time),
Q(event_end_datetime__gt=current_time),
],
{"event_status": "ongoing"},
{"status": "booked"},
)

def get_complete_reservation():
"""
Update reservations and sensors
:return:
"""
return filter_and_update_events(
[
Q(reservation_begin_datetime__lt=current_time),
Q(reservation_end_datetime__lte=current_time),
],
{"reservation_status": "complete"},
{"status": "free"},
)


Notice that I used setattr docs to set the attributes of the event and the venue. Also notice that my new function has a bit of duplication in the loops, but that is the easiest way to deal with the fact that venue is inside event and there is no obvious way to use setattr to deal with the nesting. If you need to expand your function to update even more things inside event, then I would also recommend creating a helper function that takes an event and a "setting name" and sets it, so that filter_and_update_events doesn't need to take one dictionary per object inside event.

Does this make sense?

• If there is already an answer, it might be better not to edit the question, especially the code in the question since everyone needs to be able to see the code as the first reviewer saw it. Apr 26, 2021 at 12:18
• This makes sense and works like a charm, thankyou @RGS, just add .index() during filter so as to avoid the unpacking elements error. for attr, new_value in event_changes.index(): Apr 27, 2021 at 8:04
• @pythonista woops that slipped, I meant for attr, new_value in event_changes.items():, and similarly for the venues, sorry for that. Answer has been fixed.
– RGS
Apr 27, 2021 at 8:37
• oops meant .items(), thanks for the fix Apr 29, 2021 at 11:30

Duplication could be a non-problem, but is often an alarm that something is misplaced.

This repeated piece of code

    for event in completed_events:
event.reservation_status = 'complete'
event.save()

event.venue.status = 'free'
event.venue.save()
reserve_data = dict()
reserve_data["sensor_id"] = event.venue.sensor_id
reserve_data["status"] = event.venue.status
return reserve_data


Is problematic because the action to be taken when an even is ongoing/reserved/etc should be performed by the event itself; that's why the status and the venue objects are inside the event, because they are in its domain. I see that you already have a Events class, maybe there is also an Event class?. I suggest you to have a Event class that would do something like this

# Perhaps this constants could be stored in the Events class?
ONGOING = 'ongoing'
COMPLETE = 'complete'
VENUES_STATUS = {
ONGOING: 'booked',
COMPLETE: 'free'
}

class Event:
def trigger(self, new_status, save=True):
if new_status == ONGOING:
self.event_status = ONGOING
elif new_status == COMPLETE:
self.reservation_status = COMPLETE
else:
pass  # you may have other statuses here?
if new_status in VENUES_STATUS:
self.venue.status = VENUES_STATUS[new_status]
if save:
self.save()
self.venue.save()

def data(self): # this could be moved to the Venue class, also a better name can be definitely found.
return {
"sensor_id": self.venue.sensor_id,
"status": self.venue.status
}


Having that the loop becomes (Here I'm guessing what you're trying to collect, because in your code you have a return inside the for loop; that would always return the first event data.)

def get_complete_reservation():
completed_events = Events.objects.filter(
Q(reservation_begin_datetime__lt=current_time),
Q(reservation_end_datetime__lte=current_time),
)
data = []

for event in completed_events:
event.trigger(ONGOING)
data.append(event.data())
return data



The name of the function doesn't really say what you're doing, your not just "getting" the events, you are triggering a transaction of status from one to another, so I would rewrite that like this:

def trigger_new_status(selected_events, new_status):
for event in selected_events:
event.trigger(new_status)
return [event.data() for event in selected_events]

def main():
completed_events = Events.objects.filter(
Q(reservation_begin_datetime__lt=current_time),
Q(reservation_end_datetime__lte=current_time),
)
ongoing_events = Events.objects.filter(
Q(event_begin_datetime__lte=current_time),
Q(event_end_datetime__gt=current_time),
)
completed_events_data = trigger_new_status(completed_events, COMPLETE)
ongoing_events_data = trigger_new_status(ongoing_events, ONGOING)



Another thing: if you have the possibility to modify the filter method, it would be better to use a functional approach:

def filter(self, filter_function):
return [event for event in self.events if filter_function(event)]


and that would be used like

events.filter(lambda e: e.reservation_end < today and e.reservation_begin> today)



although your code has duplication, but it is also modular. Depending on code functionality, you can trade-off this modularity with less duplication.

how about merging the 2 functions and returning two lists like as follows? this will remove code duplication, however, the trade-off would be less code-modularity.

Also, rather than using a dictionary to store booked and free sensor ids, you can just use lists with descriptive names as I have done.

def get_booked_and_free_events():
ongoing_events = Events.objects.filter(
Q(event_begin_datetime__lte=current_time),
Q(event_end_datetime__gt=current_time),
)
completed_events = Events.objects.filter(
Q(reservation_begin_datetime__lt=current_time),
Q(reservation_end_datetime__lte=current_time),
)
for event in ongoing_events:
event.event_status = 'ongoing'
event.save()

event.venue.status = 'booked'
event.venue.save()

booked_sensors = []
booked_sensors.append(event.venue.sensor_id)

for event in completed_events:
event.reservation_status ='complete'
event.save()

event.venue.status = 'free'
event.venue.save()

free_sensors = []
free_sensors.append(event.venue.sensor_id)

return booked_sensors, free_sensors
`