Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(naming): add support for roman numbers and jr suffix in extract_first_last_name/1 #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .tool-versions
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
elixir 1.16.1-otp-26
erlang 26.2.1
nelsonmestevao marked this conversation as resolved.
Show resolved Hide resolved
43 changes: 40 additions & 3 deletions lib/ex_toolkit/naming.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ defmodule ExToolkit.Naming do
returns results in a formatted, readable way.
"""

alias ExToolkit.Roman

@doc """
Shortens the first name to its initial, while preserving the rest of the name.

Expand Down Expand Up @@ -121,6 +123,21 @@ defmodule ExToolkit.Naming do

iex> Naming.extract_first_last_name("john doe smith")
"John Smith"

iex> Naming.extract_first_last_name("john doe smith jr")
"John Smith Jr"

iex> Naming.extract_first_last_name("john jr")
"John Jr"

iex> Naming.extract_first_last_name("john jose doe III")
"John Doe III"

iex> Naming.extract_first_last_name("john doe v")
"John Doe V"

iex> Naming.extract_first_last_name("Sir Alexander Chapman Ferguson")
"Sir Alexander Ferguson"
"""
@spec extract_first_last_name(nil | String.t()) :: String.t()
def extract_first_last_name(name) when is_nil(name) or name == "", do: ""
Expand All @@ -134,9 +151,29 @@ defmodule ExToolkit.Naming do
|> Enum.map(&String.capitalize/1)

case names do
[] -> ""
[first] -> first
[first | rest] -> first <> " " <> Enum.at(rest, -1)
[] ->
""

[first] ->
first

[first | rest] ->
cond do
length(rest) == 1 ->
first <> " " <> Enum.at(rest, -1)

first == "Sir" ->
"Sir" <> " " <> Enum.at(rest, 0) <> " " <> Enum.at(rest, -1)

Enum.at(rest, -1) == "Jr" ->
first <> " " <> Enum.at(rest, -2) <> " Jr"

Roman.is_valid_roman(Enum.at(rest, -1)) ->
first <> " " <> Enum.at(rest, -2) <> " " <> String.upcase(Enum.at(rest, -1))

true ->
first <> " " <> Enum.at(rest, -1)
end
end
end

Expand Down
20 changes: 20 additions & 0 deletions lib/ex_toolkit/roman.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule ExToolkit.Roman do
Copy link
Member

@nelsonmestevao nelsonmestevao Feb 10, 2024

Choose a reason for hiding this comment

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

Suggested change
defmodule ExToolkit.Roman do
defmodule ExToolkit.NumeralSystems.Roman do

And then we can replace is_valid_roman/1 with just valid?/1, what do you think?

@valid_romans ["I", "V", "X", "L", "C", "D", "M"]

def is_valid_roman(string) do
Copy link
Member

@nelsonmestevao nelsonmestevao Feb 10, 2024

Choose a reason for hiding this comment

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

Suggested change
def is_valid_roman(string) do
@spec is_valid_roman(String.t()) :: boolean()
def is_valid_roman(string) when is_binary(string) do

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget documentation and unit tests ☝️

string
|> String.upcase()
|> String.graphemes()
|> is_valid_roman(@valid_romans)
Copy link
Member

Choose a reason for hiding this comment

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

This does not make sense since you are using directly @valid_romans in the bottom case. You just need a guard.

end

defp is_valid_roman([], _), do: true

Comment on lines +11 to +12
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defp is_valid_roman([], _), do: true

defp is_valid_roman([head | tail], valid_romans) do
if Enum.member?(@valid_romans, head) do
is_valid_roman(tail, valid_romans)
else
false
end
end
Comment on lines +13 to +19
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defp is_valid_roman([head | tail], valid_romans) do
if Enum.member?(@valid_romans, head) do
is_valid_roman(tail, valid_romans)
else
false
end
end
defp is_valid_roman(graphenes) when is_list(graphenes) do
Enum.all?(graphenes, &(&1 in @valid_romans))
end

Copy link
Member

Choose a reason for hiding this comment

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

And then you can just pipe it in the original case without the extra aux function 💡

end