tommy

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

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

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 :wink:

  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
tommy

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.

eksperimental

eksperimental

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 renameinsert_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!

Where Next?

Popular in Questions Top

Tee
can someone please explain to me how Enum.reduce works with maps
New
itssasanka
Hi all, Trying to get some more clarity over utc_datetime and naive_datetime for Ecto: https://hexdocs.pm/ecto/Ecto.Schema.html#module-...
New
mcarvalho
What is the difference between System.get_env and Application.get_env? For example, what are best practices to use one versus another.
New
aadeshere1
I have a another noob question about loop. Since elixir is immutable, while loop is not directly possible. total = 10 while total != 0 ...
New
qwerescape
Is there a way to get the call stack or stack trace at any point in the code? Not from exceptions, but an expression that returns how the...
New
shahryarjb
Hello, I have map which I want to convert it to string like this: the map: %{last_name: "tavakkoli", name: "shahryar"} the string I ne...
New
fireproofsocks
Forgive me if this is obvious, but how does one delete a database record WITHOUT selecting it first? https://hexdocs.pm/ecto/Ecto.Repo.h...
New
ovidiubadita
Hey all, I discovered Elixir and I love it. I always wanted to learn a functional programming and I intended to go for Haskell, but afte...
New
beno
I will often find my self writing things similar to: case some_value do nil -&gt; something() "" -&gt; something() _ -&gt; someth...
New
wernerlaude
In AR this is so simple @articles = current_user.articles How to do in Ecto? def index(conn, _params) do current_user = conn.assig...
New

Other popular topics Top

JakeBecker
TL;DR: I’ve just released an implementation of Microsoft’s IDE-independent Language Server Protocol for Elixir. It adds language support ...
1140 51847 244
New
axelson
This post is a wiki (feel free to hit the edit button near the bottom right of this post to add your own changes!) This post collects co...
239 45766 226
New
KronicDeth
Elixir plugin for JetBrain’s IntelliJ Platform (including Rubymine) This is a plugin that adds support for Elixir to JetBrains IntelliJ...
289 35421 110
New
minhajuddin
I have seen a lot of code which picks the first element from a list using Enum.at(0) instead of List.first. Is there a reason why people ...
New
rms.mrcs
Hi, I need to transform a list of numbers into a map where the keys are the indexes and the values are the original values of the list....
New
shahryarjb
Hello, I have map which I want to convert it to string like this: the map: %{last_name: "tavakkoli", name: "shahryar"} the string I ne...
New
electic
Hi, I am new to Elixir. I am trying to use the DateTime component to insert a date into MySQL however the there seems to be no way to fo...
New
Qqwy
Original source of discussion: This topic on the Pragmatic Programmers' Functional Web Development with Elixir, OTP, and Phoenix forum. ...
New
skosch
To my knowledge, put_in, Map.update etc. all have the one limitation of not automatically creating intermediate keys when needed (for exa...
New
lanycrost
Hi everyone! I need implement if…else if…else condition from my elixir code, and anymore of this control flow structures not work proper...
New

We're in Beta

About us Mission Statement