1
\$\begingroup\$

I have a list of Workshops, each having open days (Monday, Tuesday etc.) and open time and close time (which will be same for each day). Now, Based on the current time, I need to find out next 7 days available slots. A slot is of 2 hour and is available if any of the workshop is open in that 2 hour time. The first slot of each day will start at the open_time (nearest to hour time for eg. if open_time is 09:23:54 then first slot start time will be 10:00:00) of workshop opens first on that day.

My solution so far is to have a Workshop table which has off_days, open_at, close_at, name columns. There is another table which is WorkshopHolidays which has holiday_date, workshop_id.

class Workshop(models.Model):
    DAY_CHOICES = (
        (0,'Monday'),
        (1,'Tuesday'),
        (2,'Wednesday'),
        (3,'Thursday'),
        (4,'Friday'),
        (5,'Saturday'),
        (6,'Sunday')
    )
    name = models.CharField(max_length=255,null=True)
    address = models.CharField(max_length=1024)
    open_at = models.TimeField(null=True, blank=True)
    close_at = models.TimeField(null=True, blank=True)
    off_days = MultiSelectField(choices=DAY_CHOICES,null=True,blank=True)
    active = models.BooleanField(default=True)


class WorkshopHoliday(CreatedAtAbstractBase):
    workshop = models.ForeignKey(Workshop)
    holiday_date = models.DateField()
    active = models.BooleanField(default=True)

Now given a datetime, I need to find what are the two hour slots available for next seven days. If any one store is open in a slot, that slot should be part of the output.

The way I am approaching the problem now is following:

def get_slots():
    current_time = timezone.now()
    workshops = Workshop.objects.all()
    slots = {}
    for i in range(1,7):
        curdate = current_time.date()
        day = current_time.weekday()
        curdate_dict = {'start_time':None, 'end_time':None}
        for workshop in workshops:
            off_days = workshop.off_days if workshop.off_days else ''
            if day in off_days:
                continue
            wh = WorkshopHoliday.objects.filter(holiday_date=curdate,active=True)
            if wh:
                continue
            start_time = curdate_dict.get('start_time')
            if start_time:
                if start_time > current_time.time() and start_time > workshop.open_at and workshop.open_at > current_time.time():
                    start_time = workshop.open_at
            elif workshop.open_at > current_time.time():
                    start_time = workshop.open_at
            curdate_dict['start_time'] = start_time
            end_time = curdate_dict.get('end_time')
            if end_time:
                if end_time > current_time.time() and end_time < workshop.close_at and workshop.close_at > current_time.time():
                    end_time = workshop.open_at
            elif workshop.open_at > current_time.time():
                    end_time = workshop.open_at
            curdate_dict['end_time'] = end_time
        slots[curdate] = curdate_dict
        current_time.replace(hour=0,minute=0,second=0)
        current_time = current_time + timezone.timedelta(days=1)

Please suggest if there could be a better way to approach this problem (may be a better SQL query or algorithm). Also, please point out if there is some scenario in which this will give incorrect result.

\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mathias Ettinger Apr 29 '16 at 8:39
  • \$\begingroup\$ As mentioned by Matthias Ettinger already: Please do not update your code in answered questions. Instead you may want to ask a new question \$\endgroup\$ – Vogel612 Apr 29 '16 at 9:02
  • \$\begingroup\$ @Vogel612 I thought I can add the updated code separately. \$\endgroup\$ – Anuj Apr 29 '16 at 9:09
  • \$\begingroup\$ @Anuj If you want to post an updated code, you can either post an answer explaining what you changed and how it improves upon the original; or post a new question with the new code and indicate that it is a follow-up. \$\endgroup\$ – Mathias Ettinger Apr 29 '16 at 9:17
  • 2
    \$\begingroup\$ (thought I can add the updated code [in a separate code block] Not in a non-new question, and never after the first substantial answer. Besides, it is difficult for a casual reader to interpret way I am approaching the problem now. Accepted are links to web code repositories, self-answers and, after a suitable amount of time (a week?) and/or changes, a cross-linked follow-up question.) \$\endgroup\$ – greybeard Apr 29 '16 at 9:18
1
\$\begingroup\$

You can simplify the creation of DAY_CHOICES using the calendar module:

DAY_CHOICES = tuple(enumerate(calendar.day_name))

this has the advantage of using the right locale, if need be.


You can reduce going back and forth between Python and your SQL backend using prefetching of objects, instead of querying each time you need something. Django offers the prefetch_related operation:

workshops = Workshop.objects.prefetch_related(
    Prefetch(
        'workshopholiday_set',
        queryset=WorkshopHoliday.objects.filter(
            holiday_date__ge=start_date,
            holiday_date__le=start_date + timezone.timedelta(days=7),
            active=True),
        to_attr='holidays_for_the_week'))

Where start_date is timezone.now().date(). This way, you get a list of every WorkshopHoliday for the upcoming week associated to the current workshop into workshop.holidays_for_the_week. You can test if the current date is in there with:

if curdate in {h.holiday_date for h in workshop.holidays_for_the_week}:
    continue

I would also advise to exchange the two for loops so you don't compute the set of holiday dates seven time for the same workshop.


curdate_dict is not necessary. You can replace

curdate_dict = {'start_time': None, 'end_time': None}

by

start_time, end_time = None, None

and remove all curdate_dict.get(…) and curdate_dict[…] = …. You just need to slots[curdate] = {'start_time': start_time, 'end_time': end_time} then, which will improve the memory management as well.


current_time.replace(hour=0, minute=0, second=0)

does nothing: the replace method returns a new datetime instance and since you’re not assigning it, you’re just wasting time.


for i in range(1, 7):

First of, i is never used: you should write _ instead to indicate that. Second, range(1, 7) produces 6 values. So you will perform your computations for today and the 5 next days. If you wished to compute your hours for a whole week, you should have written

for _ in range(7):

instead.

I also advise you to get the "counter" near that line. This is so the reader can grasp your intent more easily:

def get_slots():
    starting_time = timezone.now()
    start_date = starting_time.date()
    workshops = Workshop.objects.prefetch_related(
        Prefetch(
            'workshopholiday_set',
            queryset=WorkshopHoliday.objects.filter(
                holiday_date__ge=start_date,
                holiday_date__le=start_date + timezone.timedelta(days=7),
                active=True),
            to_attr='holidays_for_the_week'))
    slots = {}
    for day in range(7):
        current_time = starting_time + timezone.timedelta(days=day)
        …
\$\endgroup\$
  • \$\begingroup\$ workshop.holidays_for_the_week is a list. So cannot apply filter to it. \$\endgroup\$ – Anuj Apr 29 '16 at 4:40
  • \$\begingroup\$ @Anuj Oh, sorry about that, my django is a bit old. Will adapt the answer shortly. Anyway you should be able to transform that list of WorkshopHoliday into a list of their holiday_date and use if curdate in holidays_for_the_week. \$\endgroup\$ – Mathias Ettinger Apr 29 '16 at 5:55
  • \$\begingroup\$ Thanks. Anyway, do you have any suggestions about the algorithm. May be you can suggest a better one. \$\endgroup\$ – Anuj Apr 29 '16 at 6:30
  • \$\begingroup\$ @Anuj I tried to understand exactly what you were trying to achieve but failed. Maybe you could add this kind of detail into your question to help. As far as I got, you’re trying to compute the range of hours when every workshop is open for the upcomming week. But I don't quite get the requirement for current_time.time() comming into play. \$\endgroup\$ – Mathias Ettinger Apr 29 '16 at 8:05
  • \$\begingroup\$ I have updated the code. I have a list of Workshops, each having open_days (Monday, Tuesday etc.) and open_time and close_time (which will be same for each day). Now, Based on the current time, I need to find out next 7 days available slots. A slot is of 2 hour and is available if any of the workshop is open in that 2 hour time. The first slot of each day will start at the open_time (nearest to hour time for eg. if open_time is 09:23:54 then first slot start time will be 10:00:00) of workshop opens first on that day. How to find all the available slots? \$\endgroup\$ – Anuj Apr 29 '16 at 8:32

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.