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
# What about reservation_status ?
elif new_status == COMPLETE:
self.reservation_status = COMPLETE
# What about event_status ?
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
}
(Comments are more for you than the real code comments).
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)