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]

Question

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
```

Answer 1

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?

Answer 2

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)

Answer 3

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