Help me rewrite these nested reductions?

Questions / Help
,
1

I have a form with arbitrary levels of deeply nested associations. One particular association that lives a couple of levels down can have an upload associated with it. To track these I need to allow_upload based on the indices of each level. Currently this looks like this:

socket.assigns.form.source
|> Ecto.Changeset.get_assoc(:mockups)
|> Enum.with_index()
|> Enum.reduce(socket, fn {mockup, mockup_index}, socket ->
  mockup
  |> Ecto.Changeset.get_assoc(:elements)
  |> Enum.with_index()
  |> Enum.reduce(socket, fn {element, element_index}, socket ->
    element
    |> Ecto.Changeset.get_assoc(:transformations)
    |> Enum.with_index()
    |> Enum.reduce(socket, fn {_transformation, transformation_index}, socket ->
      allow_image_upload(
        socket,
        "transformation-#{mockup_index}-#{element_index}-#{transformation_index}"
      )
    end)
  end)
end)

I’m wondering how others would go about this.

Of course I realize I can abstract out the commonalities into a private function, but I would rather not do that as this is the only place this type of thing is happening and feel it’ll make it less readable for passers-by. I’m wondering if there is a reducer pattern that can do this type of nested thing in a flat pipeline. I have a feeling Pathex can probably come in handy here but interested in other options. I’m thinking of reducing into a 2-tuple of {socket, %{}} and building up the indices in the map then doing a final reduction to build up all the keys and allow_upload them, but that feels like it will be more complex to read.

Anyway, just wondering! As verbose as my example is, I don’t hate it as it’s pretty easy to read. Credo is complaining about the indentation depth, though, and I don’t necessarily disagree with it.

1 Like
2

When you start dealing with nested iterative structures, for is your best bet.

assoc_with_index = fn changeset, assoc ->
  changeset
  |> Ecto.Changeset.get_assoc(assoc)
  |> Enum.with_index()
end

source = socket.assigns.form.source

for {mockup, mockup_index} <- assoc_with_index.(source, :mockups),
    {element, element_index} <- assoc_with_index.(mockup, :elements),
    {_transformation, transformation_index} <- assoc_with_index.(element, :transformations) do
  allow_image_upload(
    socket,
    "transformation-#{mockup_index}-#{element_index}-#{transformation_index}"
  )
end
3 Likes
3

It also supports reduce, to allow wiring the socket state through.

3 Likes
4

It’s a great tool, one of the main reasons I like Elixir.

Yes, if you need to update the socket, then you can tack on reduce:

assoc_with_index = fn changeset, assoc ->
  changeset
  |> Ecto.Changeset.get_assoc(assoc)
  |> Enum.with_index()
end

source = socket.assigns.form.source

for {mockup, mockup_index} <- assoc_with_index.(source, :mockups),
    {element, element_index} <- assoc_with_index.(mockup, :elements),
    {_transformation, transformation_index} <- assoc_with_index.(element, :transformations),
    reduce: socket do
  socket ->
    allow_image_upload(
      socket,
      "transformation-#{mockup_index}-#{element_index}-#{transformation_index}"
    )
end
7 Likes
5

Beautiful! So much nicer than what I was thinking.

Thank you both!

2 Likes
6

This brings me joy! The for comprehension is so nice.

1 Like
7

I would also consider an approach where I would break this down into small functions, so the main function eould be flat.

2 Likes
8

I would do that if this became a common pattern. As it stands this is the only place in the app where I’m needing to index associations in order to enable uploads and this logic is already inside a private function, so I don’t want to break things up too much. Part of me actually still prefers my original example because it’s so stupidly simple, but I do appreciate the Credo rule and if I allow myself to up the nesting limit “just this one time”, there will definitely be several future instances of “Ok, this time is for sure the last time!”—I’m weak so I prefer to stick with defaults :smiling_face_with_tear:

9

FWIW the for variant, while shorter, is absolutely opaque and is making it pretty hard to mentally parse and understand what is going on.

I see nothing wrong with your original function but if you really have to act I would either (a) add a Credo exception or (b) make two sub-functions – corresponding to your two sub-levels.

I’d opt for sub-functions. Granted they’ll ruin the nice readability your original long function has but hey, if they are directly below it it should be pretty straightforward to read and grok.

1 Like
10

Ya, I might revert to my original version and completely kill the Credo rule as indenting too deeply isn’t something that happens often (I’m ok with outright removal of linting rules). The more I think about it, it’s really a pretty useless rule unless you are working with an army of developers you can’t trust. My other thought was that this feels like code that someone in the future will probably come along and rewrite anyway :sweat_smile:

I actually think the for version is quite readable and certainly very “pretty”, but I agree the original is more readable even if “ugly”. I do actually always use for whenever I need a Cartesian product of two collections but for some reason as soon as I needed the product of three I was like, “duhhhhhhhhhhh what do I do better ask the internet!”. Maybe that is good signal for me not to use it here, haha. Not to shit on anyone who prefers the for!

Anyway, one thing is for sure that I will definitely choose one of these options hopefully without thinking too much more about it :upside_down_face: :upside_down_face: :upside_down_face:

Thanks for the input, all!

1 Like
11

I agree that for can be slightly “prettier” than a chain of Enum.reduce-s. If you find that’s the case as well then I would try and make the sub-function version and just compare side by side. Informed choice and all. :slightly_smiling_face:

But yeah, I wasn’t claiming what I said as a fact, I was stating my quick intuitive impression. I learned to trust my gut so I am sharing what would I do.

Obviously go for the variant that makes it least likely to hate yourself when you revisit this code in the future.

1 Like
12

Yep, basically this. Working solo has actually given me the luxury to carve out some time to go back and re-read stuff I wrote a week ago and see how quickly I can understand it. I’ll see!

1 Like
13

Thinking about this again, I realized that at least for me, the complexity comes from the reduce, not the rest. Consider this change to the for version:

names =
  for {mockup, mockup_index} <- assoc_with_index.(source, :mockups),
      {element, element_index} <- assoc_with_index.(mockup, :elements),
      {_transformation, transformation_index} <- assoc_with_index.(element, :transformations) do
    "transformation-#{mockup_index}-#{element_index}-#{transformation_index}"
  end

socket = Enum.reduce(names, socket, &allow_image_upload(&2, &1))

I would say that the for itself is perfectly fine. The reduce part still takes some time to understand, even when it’s in its simplest form. I would argue this is because reduce is simply the wrong way around, twice, but in any case, the building of the names does not seem to be the problem at all.

1 Like
14

I actually quite like that version too! I don’t really mind the for reduce either, though, I think depends on how comfortable with for and its options you are. Some people are all about comprehensions and some people won’t touch them.

I should clarify that I like my original in this case. If I were using assoc_with_index in other parts of the system then I would without question extract it to a helper function. But this case is an anomaly in my app and one that probably won’t change often and probably never be read much. I’m trying to think of someone coming across this and needing to understand it as quickly as possible before moving on. Of course that is very tricky to gauge. It’s almost as if writing good code is hard :sweat_smile:

15

Personally, I probably wouldn’t bother looking into assoc_with_index, the usage makes pretty clear what it does. I’m also not saying that I would necessarily rewrite the code like this, I just wanted to show that the reduce was causing the most complexity here, in my opinion.

I guess my issue with the original code is that you need to thread the socket through all the layers in order to use it in the innermost reduce. That complicates it a lot for me, because the creation of the names and modification of the socket are mixed together. So maybe a version with a few flat_maps that generate the values first and then a reduce - basically my for example, but without the for - would be a pretty good solution as well.

2 Likes