Not a Code Review but asking Suggestions

Questions / Help
1

I have 2 code snippets that actually do the same job.

  1. is a bit complicated but solve the problem
  2. is bt cleaner and solve the same problem

I have this test data set.

[
  %{day: "mon", since: ~T[01:00:00], till: ~T[01:55:00]},
  %{day: "mon", since: ~T[02:00:00], till: ~T[02:55:00]},
  %{day: "mon", since: ~T[03:00:00], till: ~T[03:55:00]},
  %{day: "tue", since: ~T[01:00:00], till: ~T[01:55:00]},
  %{day: "tue", since: ~T[02:00:00], till: ~T[02:55:00]},
  %{day: "tue", since: ~T[03:00:00], till: ~T[03:55:00]},
  %{day: "wed", since: ~T[01:00:00], till: ~T[01:55:00]},
  %{day: "wed", since: ~T[02:00:00], till: ~T[02:55:00]},
  %{day: "wed", since: ~T[03:00:00], till: ~T[03:55:00]},
  %{day: "thu", since: ~T[01:00:00], till: ~T[01:55:00]},
  %{day: "thu", since: ~T[02:00:00], till: ~T[02:55:00]},
  %{day: "thu", since: ~T[03:00:00], till: ~T[03:55:00]},
  %{day: "fri", since: ~T[01:00:00], till: ~T[01:55:00]},
  %{day: "fri", since: ~T[02:00:00], till: ~T[02:55:00]},
  %{day: "fri", since: ~T[03:00:00], till: ~T[03:55:00]},
  %{day: "sat", since: ~T[01:00:00], till: ~T[01:55:00]},
  %{day: "sat", since: ~T[02:00:00], till: ~T[02:55:00]},
  %{day: "sat", since: ~T[03:00:00], till: ~T[03:55:00]},
  %{day: "sun", since: ~T[01:00:00], till: ~T[01:55:00]},
  %{day: "sun", since: ~T[02:00:00], till: ~T[02:55:00]},
  %{day: "sun", since: ~T[03:00:00], till: ~T[03:55:00]}
]

Problem: I want to show you the first available time from the above timeslots to meet me depending on today’s day.

First solution bit complex.

defmodule TestAvailabilityHours do

  @days ["mon", "tue", "wed", "thu", "fri", "sat", "sun"]
  @days_with_indexes Enum.zip(1..7, @days) |> Map.new()

  def days(), do: @days

  def days_with_indexes(), do: @days_with_indexes

  def get_first_available() do
    doctors_availability_hours = dummy_timeslots()

    case length(doctors_availability_hours) > 0 do
      true ->
        current_day =
          DateTime.utc_now()
          |> get_day_name()

        first_timeslot =
          doctors_availability_hours
          |> Enum.filter(&(&1.day == current_day))
          |> no_day_present(doctors_availability_hours, current_day)
          |> get_datetime_from_hour()

        {:ok, first_timeslot}

      _ ->
        {:error, :no_timeslots_found}
    end
  end

  defp no_day_present([], hours, day) do
    hours
    |> Enum.chunk_every(2, 1, :discard)
    |> Enum.reverse()
    |> Enum.find_value(fn [%{day: d}, next] -> if d == day, do: next end)
    |> no_more_days_left(hours)
  end

  defp no_day_present(hour, _, _), do: hour |> List.first()

  defp no_more_days_left(nil, hours), do: hours |> List.first()
  defp no_more_days_left(hour, _hours), do: hour

  defp get_datetime_from_hour(hour) do
    Enum.reduce_while(1..7, Date.utc_today(), fn _day, acc ->
      current_day =
        acc
        |> get_day_name()

      if current_day != hour.day,
        do: {:cont, Timex.shift(acc, days: 1)},
        else: {:halt, acc}
    end)
    |> NaiveDateTime.new(hour.since)
    |> elem(1)
    |> DateTime.from_naive!("Etc/UTC")
  end

  defp get_day_name(datetime) do
    datetime
    |> Timex.weekday()
    |> Timex.day_shortname()
    |> String.downcase()
  end

  def dummy_timeslots() do
    Enum.flat_map(1..7, fn day ->
      Enum.map(1..3, fn hour ->
        since = "0#{hour}:00:00"
        till = "0#{hour}:55:00"

        %{
          day: days_with_indexes()[day],
          since: Time.from_iso8601!(since),
          till: Time.from_iso8601!(till)
        }
      end)
    end)
  end
end

and now 2nd solution.

defmodule TestAvailabilityHours do

  @days ["mon", "tue", "wed", "thu", "fri", "sat", "sun"]
  @days_with_indexes Enum.zip(1..7, @days) |> Map.new()

  def days(), do: @days

  def days_with_indexes(), do: @days_with_indexes

  def test() do
    datetime = DateTime.utc_now
    weekday = Date.day_of_week(datetime)
    time = DateTime.to_time(datetime)

    availability_timeslots = dummy_timeslots()

    closest_timeslot =
      Enum.find(availability_timeslots, fn timeslot ->
        timeslot_day_index = Timex.day_to_num(timeslot.day)

        cond do
          timeslot_day_index == weekday ->
            time >= timeslot.since && time < timeslot.till

          timeslot_day_index > weekday ->
            true

          timeslot_day_index < weekday ->
            false
        end
      end)

    next_monday_beginning =
      datetime |> Timex.end_of_week() |> Timex.shift(seconds: 1)

    next_closest_timeslot = List.first(availability_timeslots)

    timeslot_to_datetime(closest_timeslot, datetime) ||
      timeslot_to_datetime(next_closest_timeslot, next_monday_beginning)
  end

  def timeslot_to_datetime(nil, _), do: nil

  def timeslot_to_datetime(timeslot, datetime) do
    weekday_index = Date.day_of_week(datetime)
    date = DateTime.to_date(datetime)

    timeslot_day_index = Timex.day_to_num(timeslot.day)

    cond do
      weekday_index == timeslot_day_index ->
        NaiveDateTime.new(date, timeslot.since)

      weekday_index < timeslot_day_index ->
        date = Timex.shift(date, days: timeslot_day_index - weekday_index)
        NaiveDateTime.new(date, timeslot.since)
    end
  end

  def dummy_timeslots() do
    Enum.flat_map(1..7, fn day ->
      Enum.map(1..3, fn hour ->
        since = "0#{hour}:00:00"
        till = "0#{hour}:55:00"

        %{
          day: days_with_indexes()[day],
          since: Time.from_iso8601!(since),
          till: Time.from_iso8601!(till)
        }
      end)
    end)
  end
end

I already know the 2nd solution is the better one. But can you suggest to me if I can still do better with it? is there any more easy way than this to solve this problem?

also how I can test both methods with regards to time? I mean which is more performant?

2

Well, not sure whether this would be helpful but anyway.
You could have the days as your keys in the map so you would only have 7 different keys.

%{
  mon: [%{since: ..., till: ...}],
  tue: [],
  ...
}

That way you could get a specific day easier and from there you can work out the first available time.
Maybe even better with indexes so you can only look for indexes that are greater than or equal to today’s index.

1 Like
3

I’ve only given a very quick look, but you can eliminate some nesting from your dummy_timeslots/0 function with a list comprehension.

def dummy_timeslots do
  for day <- 1..7, hour <- 1..3 do
    since = "0#{hour}:00:00"
    till = "0#{hour}:55:00"

    %{
      day: days_with_indexes()[day],
      since: Time.from_iso8601!(since),
      till: Time.from_iso8601!(till)
    }
  end
end
4

I think you are overly complicating yourself.
I think your dummy data should be a list with all the available DateTimes and take out the logic of that part of your code. If there is any logic use it for building that data, but in real life that will come from the database where you store your appointments.
Then you just do an Enum.find where the datetime >= now.

5

I found a very simple solution just now.

defmodule TestAvailabilityHoursSimple do
  alias Onemedical.DoctorAvailabilityHours

  #these will be remove as well
  
  @days ["mon", "tue", "wed", "thu", "fri", "sat", "sun"]
  @days_with_indexes Enum.zip(1..7, @days) |> Map.new()

  def days(), do: @days

  def days_with_indexes(), do: @days_with_indexes
  #these will be remove as well
  
  def get_available_timeslot() do
    timeslot =
      dummy_timeslots()
      |> Enum.map(&get_datetime_from_hour(&1))
      |> Enum.sort()
      |> List.first()

    case timeslot do
      nil -> {:error, :no_timeslots_found}
      timeslot -> {:ok, timeslot}
    end
  end

  defp get_datetime_from_hour(hour) do
    Enum.reduce_while(1..7, Date.utc_today(), fn _day, acc ->
      current_day =
        acc
        |> get_day_name()

      if current_day != hour.day,
        do: {:cont, Timex.shift(acc, days: 1)},
        else: {:halt, acc}
    end)
    |> NaiveDateTime.new(hour.since)
    |> elem(1)
    |> DateTime.from_naive!("Etc/UTC")
  end

  defp get_day_name(datetime) do
    datetime
    |> Timex.weekday()
    |> Timex.day_shortname()
    |> String.downcase()
  end
 
  #This will be removed as well
  def dummy_timeslots() do
    Enum.flat_map(1..7, fn day ->
      Enum.map(1..3, fn hour ->
        since = "0#{hour}:00:00"
        till = "0#{hour}:55:00"

        %{
          day: days_with_indexes()[day],
          since: Time.from_iso8601!(since),
          till: Time.from_iso8601!(till)
        }
      end)
    end)
  end
end
6

your new code and I think the previous one also were giving a date in the past.
For example it was 2.30 and it was returning a date for the same day but time at 2:00

Have you considered what happens if the appointment is done after the last available slot for that week?

7

Maybe I am wrong, as it was the UTC time zone.

8

Yes, it was UTC. but thank you so much for looking at code I really appreciate this.

9

Still have a look because I think that bug happens, regardless the UTC.
Sure, no worries.

10

You were right.

I had to add this

  defp reject_old_timeslots(timeslots) do
    timeslots |> Enum.reject(&Timex.before?(&1, DateTime.utc_now()))
  end
1 Like