tommy
Can I show of my code and also get feedback? My list sort implementation
This took me all day! I know there is Enum.sort but I’m learning by implementing some of the Enum functions. I’m sure the Enum version is a lot cleaner.
This was a lot of fun! Also, have I implemented a known sorting algorithm?
defp place_in_sorted_list(list, int) do
Enum.map(list, &if(int < &1, do: true, else: false))
|> Enum.find_index(&(&1 == true))
end
defp insert_int(list, int) do
pos = place_in_sorted_list(list, int)
if pos == nil, do: list ++ [int], else: List.insert_at(list, pos, int)
end
def sort([h | t]), do: sort(t, [h])
def sort([h | t], new_list), do: sort(t, insert_int(new_list, h))
def sort([], new_list), do: new_list
##
sort([10, 1, 5, 1, 8, 3, 4, 37, 8, 2]) |> IO.inspect()
Most Liked
eksperimental
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
NobbZ
On a very first and Brief read it looks like an insertion sort, though I might be a bit out of practice, haven’t sorted by myself since university anymore ![]()
- You sort does fail on empty lists
if cond, do: true, else: falsecan be written ascond, and if you require stricttrue/falsethen!!condis idiomatic.- Instead of searching the index first and then inserting at that position, you could use recursion to find the position and insert in one go
tommy
This is great. That’s really clean code. I must admit I have a bit of an obsession with one line functions. To my dyslexic eyes your code is easy to parse with my eyes.
I can see now that:
defp place_in_sorted_list(list, int) do
Enum.map(list, &if(int < &1, do: true, else: false))
|> Enum.find_index(&(&1 == true))
end
was unnecessary. I was struggle to know how to find the index. I was kind of proud of my hack but still it felt like a hack.
I was also unhappy with the name of place_in_sorted_list. Place was meant to refer to the place in the list. I do find it hard naming functions. They can end up looking like long sentences.
I can see now that by using @spec that I can let that do the “talking” instead of the function name.
That makes sense what you say about grouping functions with the same name and arity.
I have learnt a lot from this excise and your help. Thanks for helping.
Popular in Questions
Other popular topics
Categories:
Sub Categories:
Forums
Popular Tags
- #ecto
- #liveview
- #troubleshooting
- #learning-elixir
- #deployment
- #library
- #erlang
- #testing
- #genserver
- #mix
- #absinthe
- #remote-other
- #otp
- #plug
- #how-to-question
- #macros
- #postgres
- #channels
- #elixirconf
- #exunit
- #discussion
- #code-sync
- #javascript
- #podcasts
- #onsite
- #dialyzer
- #docker
- #authentication
- #umbrella
- #full-time-contract
- #podcasts-by-brainlid
- #ecto-query
- #elixir-ls
- #phoenix_html
- #iex
- #blog-post
- #graphql
- #genstage
- #ai
- #websockets
- #supervisor
- #advent-of-code
- #elixirconf-us
- #distillery
- #processes
- #forms
- #api
- #metaprogramming
- #security
- #performance









