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()
``````
2 Likes

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

1. You sort does fail on empty lists
2. `if cond, do: true, else: false` can be written as `cond`, and if you require strict `true`/`false` then `!!cond` is idiomatic.
3. 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
4 Likes

This quick review is just on how I would write this code. I does not involve any code optimization.

``````defmodule Tommy do

# 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([], 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

# 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([], 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([], 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
``````
5 Likes

Thank you. I had a feeling that having a function just to look for the index was a bit of a unnecessary and ugly hack. I couldn’t figure out how to find the index so found myself isolating the problem.

What’s the advantage to cond over ifs? I’ll look up now and play with some code.

Thanks for you help.

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.

2 Likes

There is nothing wrong with giving long names to private functions, so long as they are descriptive it is good.

Speaking of naming, one more comment is would rename`insert_in` as `sort_insert`, when these functions are inside a module like Enum, naming then after the public function is a good way to spot them, plus `int` is not necessary, as in the future you also decide that your sort function support floats, that means you will have to refactor your code.

Yes, even though the exercise is small, there are a lot of lessons to learn specially if you are just beginning.

All the best!

1 Like

Personally I consider a single `cond` more readable then nested `if`. At least when there are more than 3 conditions (whereas I consider a “trailing” `else` a condition). And still I try to avoid boolean logic and prefer pattern matches whenever possible.

Though I was not talking about `if/2` vs `cond/1`.

I was talking about the fact, that `if whatever, do: true, else: false` is the same as `whatever` (if truthyness is fine) or `!!whatever` (if strict booleans are required).

That make sense particularly in Elixir. I love pattern matching and miss it when coding in dart.