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.

Where Next?

Popular in Questions Top

chokchit
** (DBConnection.ConnectionError) connection not available and request was dropped from queue after 2733ms. You can configure how long re...
New
nobody
How to bind a phoenix app to a specific ip address? could not find anything about that, nowhere, unfortunately, but for me this is quite...
New
greenz1
I have a phoenix application from which a user can download multiple(5-6) files of size 1MB. I couldn’t find anything related to sending ...
New
shahryarjb
Hello, I get Persian date from my client and convert it to normal calendar like this: def jalali_string_to_miladi_english_number(persi...
New
Qqwy
Original source of discussion: This topic on the Pragmatic Programmers’ Functional Web Development with Elixir, OTP, and Phoenix forum. ...
New
vrod
I am using the Starship cross-shell prompt – it seems pretty nice, but I get some errors: [WARN] - (starship::utils): Executing command ...
New
Emily
I have VueJS GUIs with the project generated using Webpack. I have Elixir modules that will need to be used by the VueJS GUIs. I forese...
New
shijith.k
I am trying to start a new phoenix project with elixir 1.9, but mix phx.new does not work. It says that ** (Mix) The task "phx.new" could...
New
hariharasudhan94
Lets say I have map like this fetching from my database %{"_id" =&gt; #BSON.ObjectId&lt;58eb1a7a9ad169198c3dXXXX&gt;, "email" =&gt; ...
New
JorisKok
I have a server on AWS, and was running a load test using artillery. When looking at the Phoenix dashboard I see the Ports going to 100% ...
New

Other popular topics Top

sen
Hi All, I set a environment variables in dev.exs , like below code. when i start server, how can i set the ${enable} value? thanks. d...
New
lastday4you
I wanted to check elixir version in phoenix because i found that my elixir is 1.5 but when i use Enum.chunk_by it said the function is un...
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
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
lessless
I believe there are people here who are dealing with CSV files import on the daily basis, and since Excel is a really popular tool there ...
New
Patoshizzle
After calling mix ecto.create I get this error: 17:00:32.162 [error] GenServer #PID&lt;0.412.0&gt; terminating ** (Postgrex.Error) FATAL...
New
JeremM34
Hello, how can I check the Phoenix version ? Thanks !
New
jason.o
In the code below, if the create action is not set to accept “extra_key” as an input, it errors out with a message shown above. Is there ...
New
boundedvariable
I am going through the kafka architecture. All the features what the kafka is providing are already in Erlang. I would like hear your opi...
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

We're in Beta

About us Mission Statement