Improve this code?

sodapopcan

I thought I would make one of these “improve this code” posts even though this problem isn’t really at that interesting (but here we are). I don’t hate my solution but I feel like it could perhaps be a little more readable. I’m not sure. It’s not bad, but maybe it could be better? Maybe there is an existing way to do this? I am aware of breadcrumble_ex but it’s too explicit for my use-case. I’d also like a better name, though not putting that on you all.

So this function takes a URI path (eg. /path/to/some/place) and turns it into a list of tuples suitable for a presenter to blindly turn it into breadcrumbs. Here it is:

defmodule ExampleModule do
  @doc """
  Takes a path and return a list of tuples in the form of:

    [{"segment", "/path/to/segment"}]

  ## Example:

    iex> ExampleModule.segment_path("/path/to/segment")
    iex> [{"path", "/path"}, {"to", "/path/to"}, {"segement", "/path/to/segment"}]

  This is useful for making breadcrumbs.
  """
  def segment_path(path) when is_binary(path) do
    path
    |> String.split("/")
    |> Enum.reverse()
    |> do_segment_path([])
  end

  defp do_segment_path([""], acc), do: acc

  defp do_segment_path([segment | rest] = path, acc) do
    path =
      path
      |> Enum.reverse()
      |> Enum.join("/")

    acc = [{segment, path} | acc]

    do_segment_path(rest, acc)
  end
end

That’s it!

Not too exciting, I know. I mostly like this little problem since this is one of those cases where building a list from the end is actually what we want to do. Of course, this just means we reverse it at the beginning as opposed to the end, and then again each iteration to put each segment in its correct form. That’s the big thing I think I would find confusing about this coming back to it later with no context.

Code-golf answers are welcome, but I’m ultimately looking for readability improvements.

18 comments

#recursion #loop

4 1849 18

2024-02-28 21:57:11 UTC

Showing Posts 18 to 9

Show Best Posts
Show All (oldest first)
Show All (newest first)

sodapopcan

I mean, ya, that one certainly works well

Post #18

smathy

Late to the party here, but I’d use the Path helpers:

  def segment_path(path) when is_binary(path) do
    do_segment_path(path)
    |> Enum.reverse()
  end

  defp do_segment_path("/"), do: []

  defp do_segment_path(path) do
    [ { Path.basename(path), path } | do_segment_path(Path.dirname(path)) ]
  end

Post #17

sodapopcan

lol, thanks for necroing this thread as made me realize I never marked a solution

Post #16

GPrimola

I loved how simple and straightforward this looks!

Post #15

cloudytoday

With a for comprehension:

iex(109)> sp = fn path ->
...(109)>   split = String.split(path, "/")
...(109)>   for el <- split do
...(109)>     path = Enum.take_while(split, & &1 != el) ++ [el]
...(109)>     {el, Enum.join(path, "/")}
...(109)>   end
...(109)> end
#Function<42.3316493/1 in :erl_eval.expr/6>
iex(110)> sp.("path/to/some/segment")
[
  {"path", "path"},
  {"to", "path/to"},
  {"some", "path/to/some"},
  {"segment", "path/to/some/segment"}
]

Post #14

Sebb

import Enum

def bread(p), do: p |> String.split("/") |> split(-1) |> bread_()

defp bread_({[], _}), do: []
defp bread_({p, [s]}), do: [{s, join(p ++ [s], "/")}] ++ (p |> split(-1) |> bread_())

bread("/path/to/segment") #=>
 [{"segment", "/path/to/segment"}, {"to", "/path/to"}, {"path", "/path"}]

this is inefficient, but easy to follow and short. I opt for easy here because this will never be time critical.
“Fast-readability” is also a kind of fast code.

Post #13

adamu

Hopefully this is at least par for the course . @Ninigi I’m interested in what you’d use instead of do_ here.

defmodule Example do
  def segment_path(path) do
    path |> String.split("/", trim: true) |> do_segment_path("")
  end

  defp do_segment_path([], _path), do: []

  defp do_segment_path([current | rest], path) do
    new_path = "#{path}/#{current}"
    [{current, new_path} | do_segment_path(rest, new_path)]
  end
end

Running:

iex(1)> Example.segment_path("path/to/segment")
[{"path", "/path"}, {"to", "/path/to"}, {"segment", "/path/to/segment"}]

Post #12

sodapopcan

In complex cases for sure—I did a little parser thing once and didn’t use do_ at all, but I honestly don’t write a lot of recursive functions. I mostly do simple cases and (and I’m entering into mega bikeshed territory here) I hate when things have names for the sake of having names… if that makes sense? Like when I see _other -> true in a catch-all or the like. I name my _ vars 99% of the time but case like that I’m like, “Duh, I know it’s ‘other’!”

I dunno, I’m procrastinating on a project right now

Post #11

Ninigi

do_ for recursive functions is something the core team has explicitly moved away from, on the base that it’s just lazy naming, and leads to move your core logic into a private function - which is like stuffing your mess into a closet, close the door and call the room “cleaned up”

My opinion on this: if you can’t come up with a name for your recursive function other than do_public_fun, then your logic should probably live in the public function.

Post #10

sodapopcan

For utility functions like these that do one very simple tiny thing that will very likely never change (and I wish were just in a library), I prefer them to be terse. When they take up a lot of lines it psychologically makes me think they are doing something really important when skimming through code. That’s just me and my personal projects, though. If I wrote that as part of a team and someone wanted me to change it, I wouldn’t protest.

If I wrote this today (this was 9 months ago… I just got pinged on this relatively old thread) I would probably expand the shorthand anonymous functions.

The do_ for private functions that recurse is a very common Elixir idiom which is why I do it. I didn’t like it at first but I really like it now as it acts as general visual “glue” for all recursive functions.

Post #9

Load more posts (8 remaining)