This quick review is just on how I would write this code. I does not involve any code optimization.
defmodule Tommy do
# First define your API, then your helpers
# While common practice with h & t, and k & v, I tried to avoid single-letter variables.
# Personally i like to break into two lines the functions, I think they are more readable.
def sort([head | tail]),
do: sort(tail, [head])
def sort([head | tail], new_list),
do: sort(tail, insert_int(new_list, head))
def sort([], new_list),
do: new_list
defp place_in_sorted_list(list, int) do
# I would not use function captures here (&), try to use pattern matching whenever possible,
list
|> Enum.map(fn
element when int < element -> true
_ -> false
end)
# since values are true/false you can just use this
|> Enum.find_index(& &1)
end
defp insert_int(list, int) do
# You can put everything in one if block.
# Also avoid one-liners when they are not that simple
if pos = place_in_sorted_list(list, int) do
List.insert_at(list, pos, int)
else
list ++ [int]
end
end
end
EDIT: Second review which is a bit more in depth
defmodule Tommy do
# First define your API, then your helpers
# While common practice with h & t, and k & v, I tried to avoid single-letter variables.
# Personally i like to break into two lines the functions, I think they are more readable.
# Specs are important do document your code and make it more understandtable to others and to your future self.
# Look in this one line you can tell what the function does. You don't even need @doc to explain it.
@spec sort(nonempty_list(integer)) :: nonempty_list(integer)
@spec sort([]) :: []
# what happens if somebody passes an empty list? your function will crash if you don't cover this case,
# so we don't do patten matching here and let the helper deal with pattern matching and empty lists as it already does.
def sort(list) when is_list(list),
do: sort(list, [])
# Separate your functions of different arities. By looking at them, all piled up in one line and no
# empy lines it lokked like they were all the same arity.
# Also this is a helper, so it can be private.
# I would rename: new_list, acc
# Another thing. I add a guard to check for integers, since I assume based on `insert_int` that
# what you want to sort is only integers
defp sort([head | tail], acc) when is_integer(head),
do: sort(tail, insert_int(acc, head))
defp sort([], acc),
do: acc
# The name of your function does not describe what is does.
# As it does not place (insert), but find/locate its index/position.
# Another note: position in Elixir are 1-based, indexes are 0-based, and they are being mixed here,
# so you should stick to one (index is the obvio choise here)
defp find_index_in_sorted_list([], _int),
do: nil
defp find_index_in_sorted_list(list, int) do
# Here you are unnecessarily travering the list, because all you want is
# to find the index of the first element where int is <
Enum.find_index(list, &(int < &1))
end
defp insert_int(list, int) do
# You can put everything in one if block.
# Also avoid one-liners when they are not that simple
if index = find_index_in_sorted_list(list, int) do
List.insert_at(list, index, int)
else
list ++ [int]
end
end
end
Tommy.sort([10, 1, 5, 1, 8, 3, 4, 37, 8, 2]) |> IO.inspect()
So without comments and removing an unnecessary helper, your code will look like:
defmodule Tommy do
@spec sort(nonempty_list(integer)) :: nonempty_list(integer)
@spec sort([]) :: []
def sort(list) when is_list(list),
do: sort(list, [])
defp sort([head | tail], acc) when is_integer(head),
do: sort(tail, insert_int(acc, head))
defp sort([], acc),
do: acc
defp insert_int(list, int) do
if index = Enum.find_index(list, &(int < &1)) do
List.insert_at(list, index, int)
else
list ++ [int]
end
end
end
