Analysis of Newsletter

[j0sht]

Hello! First thanks to all the contributors of the Elixir track at Exercism! Elixir if quickly becoming my favourite programming language :)

I just finished coding my solution to the Newsletter challenge, and here's what I submitted:

defmodule Newsletter do
  def read_emails(path), do: do_read_email(File.read!(path))
  defp do_read_email(""), do: []
  defp do_read_email(emails), do: String.trim(emails) |> String.split("\n")

  def open_log(path), do: File.open!(path, [:write])

  def log_sent_email(pid, email), do: IO.puts(pid, email)

  def close_log(pid), do: File.close(pid)

  def send_newsletter(emails_path, log_path, send_fun) do
    do_send(read_emails(emails_path), open_log(log_path), send_fun)
  end

  defp do_send([], log_pid, _), do: close_log(log_pid)

  defp do_send([email | rest], log_pid, f) do
    if f.(email) === :ok, do: log_sent_email(log_pid, email)
    do_send(rest, log_pid, f)
  end
end

The analyzer gave me the following feedback:

The function send_newsletter/3 should not explicitly return :ok, but implicitly return what close_log/1 returns (which happens to be :ok when all goes well).

However, I do actually implicitly return what close_log/1 returns with the help of the private function do_send in my solution. I'm sure there's other ways of writing a solution here, but no where do I explicitly return :ok from any function in the solution, so I'm not sure why the analyzer is claiming I have for send_newsletter/3 😕

[angelikatyborska]

Hi! Thanks for reporting.

The reason for this confusing feedback is that the analyzer expected you to call close_log as the last thing in send_newsletter. We did not foresee that people could choose a recursive approach to this exercise. Here's what we expected:

def send_newsletter(emails_path, log_path, send_fun) do
  log_pid = open_log(log_path)
  emails = read_emails(emails_path)

  Enum.each(emails, fn email ->
    case send_fun.(email) do
      :ok -> log_sent_email(log_pid, email)
      _ -> nil
    end
  end)

  close_log(log_pid)
end

@jiegillet Do you have any ideas how we could improve the current check? Do you remember why we wrote "must end with close_log" instead of "cannot end with :ok"?

[jiegillet]

I guess we should change the check to match the message of "cannot end with :ok". We made that very change for other checks, but I we didn't do this one probably because no one complained before.