Help me make this basic validation better

Questions / Help
1

I’m working in a vacuum and I’d like a little code review.

How can I refactor this and make it nicer?

In short I’m comparing a start_date and a end_date.
end_date is optional where as start date is not.
end_date can’t be before the start_date.

Best Josh

defp validate_dates(
         %Ecto.Changeset{valid?: true, changes: %{start_date: start_date, end_date: end_date}} =
           cs
       ) do
    if end_date != nil do
      compaire_dates(cs, start_date, end_date)
    else
      cs
    end
  end

  defp validate_dates(%Ecto.Changeset{valid?: true, changes: %{start_date: start_date}} = cs) do
    if cs.data.end_date != nil do
      compaire_dates(cs, start_date, cs.data.end_date)
    else
      cs
    end
  end

  defp validate_dates(%Ecto.Changeset{valid?: true, changes: %{end_date: end_date}} = cs) do
    if end_date != nil do
      compaire_dates(cs, cs.data.start_date, end_date)
    else
      cs
    end
  end

  defp validate_dates(cs), do: cs

  defp compaire_dates(cs, start_date, end_date) do
    case Timex.before?(end_date, start_date) do
      true ->
        cs |> add_error(:end_date_string, "End Date can't be before the Start Date.")

      _ ->
        cs
    end
  end
2

Maybe you can use apply_changes/1 and simplify your logic a bit then.

@spec validate_dates(Ecto.Changeset.t()) :: Ecto.Changeset.t()
defp validate_dates(changeset) do
  %Resource{start_date: start_date, end_date: end_date} = apply_changes(changeset)
  case valid_dates?(start_date, end_date) do
    true ->
      changeset
    {false, {error_key, error_msg}} ->
      add_error(changeset, error_key, error_msg)
  end
end

and

@spec valid_dates?(Date.t() | nil, Date.t() | nil) :: true | {false, {atom, String.t()}}
defp valid_dates?(nil, nil), do: {false, {:start_date, "dates can't be both nil"}}
defp valid_dates?(_date, nil), do: true
defp valid_dates?(nil, _date), do: {false, {:start_date, "start dats can't be nil"}}
defp valid_dates?(start_date, end_date) do
  case Date.compare(start_date, end_date) do
    :lt -> true
    :gt -> {false, {:end_date_string, "End Date can't be before the Start Date."}}
    :eq -> true
  end
end

I’ve removed %{valid?: true} check since I think it’s a “better” UX to return all errors at once. I only use %{valid?: true} when the validation is computationally expensive.

3 Likes
3

You can remove all if-else’s by adding another compaire_dates function that matches on a nil end_date before the current defined one

2 Likes
4 
defp validate_dates(cs) do
  with %{valid?: true} <- cs,
       end_date when not is_nil(end_date) <- get_field(cs, :end_date),
       start_date <- get_field(cs, :start_date) do
    compaire_dates(cs, start_date, end_date)
  else
    _ -> cs
  end
end

defp compaire_dates(cs, start_date, end_date) do
  case Timex.before?(end_date, start_date) do
    true -> add_error(cs, :end_date_string, "End Date can't be before the Start Date.")
    _ -> cs
  end
end
1 Like
5

I really need to take the time to start using “with”, thanks.

1 Like
6

+1 for Dialyzer specs :slight_smile:
Thats another habit I really need to start.

1 Like