Skip to content

Cleanups#11

Open
richleland wants to merge 4 commits intomasterfrom
cleanups
Open

Cleanups#11
richleland wants to merge 4 commits intomasterfrom
cleanups

Conversation

@richleland
Copy link
Copy Markdown

@richleland richleland commented May 25, 2017

  • Updated deps
  • Cleaned up warning output from mix test
  • Cleaned up warning output from mix credo
  • Added us to the maintainers list
  • Made mix docs work

email.headers
else
Map.put_new(email.headers, "CC", Enum.map(email.cc, fn({_,addr}) -> addr end) |> Enum.join(","))
cc_addrs = for {_, addr} <- email.cc, do: addr
Copy link
Copy Markdown

@genericlady genericlady May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this abstraction but why not take it a step further and turn it into a function rather than assigning value to a variable? Also, this is possibly a smell in Elixir because they don't like assigning values to variables. I believe the core team prefers smaller functions to smaller variables. Is something like this reasonable?

Also, Uncle Bob wrote Clean Code and there's Pike <> Kernighan wrote PoP (The Practice of Programming) they believe in not using abbreviations unless it's something like an intermediate variable in an Enumeration and even then it's not highly advised.

  defp email_headers(email) do
    if email.cc == [] do
      email.headers
    else
      Map.put_new(email.headers, "CC", cc_addresses(email))
    end
  end

  def cc_addresses(email)
    addresses = for {_, addr} <- email.cc, do: addr
    addresses |> Enum.join(",")
    # maybe this will work?
    # for {_, addr} <- email.cc, do: addr |> Enum.join(",")
  end

^^I didn't test if this would work but it would be something like this obviously.^^

P.S. If you abstract into a smaller function it will further clean up Enum.join(",") this can go in cc_addresses/1

Further refactoring recommended from Elixir Lang Slack
courtesy of mcelaney

defp email_headers(%{cc: []}), do: email.headers
defp email_headers(%{headers: headers} = email) do
  Map.put_new(email.headers, "CC", cc_addresses(email))
end

def cc_addresses(%{cc: {_, addresses}}), do: addresses |> Enum.join(","))
def cc_addresses(_), do: ""

This is a good pattern because the logic is cleaned up through the use of pattern matching.

end

defp add_b_cc(recipients, new_recipients, to) do
to_addrs = for {_, addr} <- to, do: addr
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

samething goes for this, I would make it a smaller function and name it after it's intent with full words like to_address/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants