refactor: reduce nesting depth and improve code readability

- Replace Enum.map |> Enum.join with Enum.map_join for efficiency
- Extract helper functions to reduce nesting depth from 4 to 2
- Rename is_current_cycle? to current_cycle? following Elixir conventions

lib/membership/member.ex

@@ -607,24 +607,27 @@ defmodule Mv.Membership.Member do
     cycles = Map.get(member, :membership_fee_cycles)
 
     if is_list(cycles) and cycles != [] do
-      Enum.find(cycles, fn cycle ->
-        case Map.get(cycle, :membership_fee_type) do
-          %{interval: interval} ->
-            cycle_end =
-              Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
-
-            Date.compare(cycle.cycle_start, today) in [:lt, :eq] and
-              Date.compare(today, cycle_end) in [:lt, :eq]
-
-          _ ->
-            false
-        end
-      end)
+      Enum.find(cycles, &current_cycle?(&1, today))
     else
       nil
     end
   end
 
+  # Checks if a cycle is the current cycle (active today)
+  defp current_cycle?(cycle, today) do
+    case Map.get(cycle, :membership_fee_type) do
+      %{interval: interval} ->
+        cycle_end =
+          Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
+
+        Date.compare(cycle.cycle_start, today) in [:lt, :eq] and
+          Date.compare(today, cycle_end) in [:lt, :eq]
+
+      _ ->
+        false
+    end
+  end
+
   @doc false
   def get_last_completed_cycle(member) do
     today = Date.utc_today()
@@ -634,32 +637,42 @@ defmodule Mv.Membership.Member do
 
     if is_list(cycles) and cycles != [] do
       cycles
-      |> Enum.filter(fn cycle ->
-        case Map.get(cycle, :membership_fee_type) do
-          %{interval: interval} ->
-            cycle_end =
-              Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
-
-            # Cycle must have ended (cycle_end < today)
-            Date.compare(today, cycle_end) == :gt
-
-          _ ->
-            false
-        end
-      end)
-      |> Enum.sort_by(
-        fn cycle ->
-          interval = Map.get(cycle, :membership_fee_type).interval
-          Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
-        end,
-        {:desc, Date}
-      )
+      |> filter_completed_cycles(today)
+      |> sort_cycles_by_end_date()
       |> List.first()
     else
       nil
     end
   end
 
+  # Filters cycles that have ended (cycle_end < today)
+  defp filter_completed_cycles(cycles, today) do
+    Enum.filter(cycles, fn cycle ->
+      case Map.get(cycle, :membership_fee_type) do
+        %{interval: interval} ->
+          cycle_end =
+            Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
+
+          Date.compare(today, cycle_end) == :gt
+
+        _ ->
+          false
+      end
+    end)
+  end
+
+  # Sorts cycles by end date in descending order
+  defp sort_cycles_by_end_date(cycles) do
+    Enum.sort_by(
+      cycles,
+      fn cycle ->
+        interval = Map.get(cycle, :membership_fee_type).interval
+        Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
+      end,
+      {:desc, Date}
+    )
+  end
+
   @doc false
   def get_overdue_cycles(member) do
     today = Date.utc_today()
@@ -668,30 +681,31 @@ defmodule Mv.Membership.Member do
     cycles = Map.get(member, :membership_fee_cycles)
 
     if is_list(cycles) and cycles != [] do
-      Enum.filter(cycles, fn cycle ->
-        case Map.get(cycle, :membership_fee_type) do
-          %{interval: interval} ->
-            cycle_end =
-              Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
-
-            cycle.status == :unpaid and Date.compare(today, cycle_end) == :gt
-
-          _ ->
-            false
-        end
-      end)
+      filter_overdue_cycles(cycles, today)
     else
       []
     end
   end
 
+  # Filters cycles that are unpaid and have ended (cycle_end < today)
+  defp filter_overdue_cycles(cycles, today) do
+    Enum.filter(cycles, fn cycle ->
+      case Map.get(cycle, :membership_fee_type) do
+        %{interval: interval} ->
+          cycle_end =
+            Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
+
+          cycle.status == :unpaid and Date.compare(today, cycle_end) == :gt
+
+        _ ->
+          false
+      end
+    end)
+  end
+
   # Regenerates cycles when membership fee type changes
   # Deletes future unpaid cycles and regenerates them with the new type/amount
   defp regenerate_cycles_on_type_change(member) do
-    alias Mv.MembershipFees.MembershipFeeCycle
-    alias Mv.MembershipFees.CycleGenerator
-    alias Mv.MembershipFees.CalendarCycles
-
     require Ash.Query
 
     today = Date.utc_today()
@@ -699,61 +713,74 @@ defmodule Mv.Membership.Member do
     # Find all unpaid cycles for this member
     # We need to check cycle_end for each cycle using its own interval
     all_unpaid_cycles_query =
-      MembershipFeeCycle
+      Mv.MembershipFees.MembershipFeeCycle
       |> Ash.Query.filter(member_id == ^member.id)
       |> Ash.Query.filter(status == :unpaid)
       |> Ash.Query.load([:membership_fee_type])
 
     case Ash.read(all_unpaid_cycles_query) do
       {:ok, all_unpaid_cycles} ->
-        # Filter cycles that haven't ended yet (cycle_end >= today)
-        # These are the "future" cycles that should be regenerated
-        # Use each cycle's own interval to calculate cycle_end
-        cycles_to_delete =
-          Enum.filter(all_unpaid_cycles, fn cycle ->
-            case cycle.membership_fee_type do
-              %{interval: interval} ->
-                cycle_end = CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
-                Date.compare(today, cycle_end) in [:lt, :eq]
-
-              _ ->
-                false
-            end
-          end)
-
-        # Delete future unpaid cycles
-        if Enum.empty?(cycles_to_delete) do
-          # No cycles to delete, just regenerate
-          case CycleGenerator.generate_cycles_for_member(member.id) do
-            {:ok, _cycles} -> :ok
-            {:error, reason} -> {:error, reason}
-          end
-        else
-          delete_results =
-            Enum.map(cycles_to_delete, fn cycle ->
-              Ash.destroy(cycle)
-            end)
-
-          # Check if any deletions failed
-          if Enum.any?(delete_results, &match?({:error, _}, &1)) do
-            {:error, :deletion_failed}
-          else
-            # Regenerate cycles with new type/amount
-            # CycleGenerator uses its own transaction with advisory lock
-            # It will reload the member, so it will see the deleted cycles are gone
-            # and the new membership_fee_type_id
-            case CycleGenerator.generate_cycles_for_member(member.id) do
-              {:ok, _cycles} -> :ok
-              {:error, reason} -> {:error, reason}
-            end
-          end
-        end
+        cycles_to_delete = filter_future_cycles(all_unpaid_cycles, today)
+        delete_and_regenerate_cycles(cycles_to_delete, member.id)
 
       {:error, reason} ->
         {:error, reason}
     end
   end
 
+  # Filters cycles that haven't ended yet (cycle_end >= today)
+  # These are the "future" cycles that should be regenerated
+  defp filter_future_cycles(all_unpaid_cycles, today) do
+    Enum.filter(all_unpaid_cycles, fn cycle ->
+      case cycle.membership_fee_type do
+        %{interval: interval} ->
+          cycle_end =
+            Mv.MembershipFees.CalendarCycles.calculate_cycle_end(cycle.cycle_start, interval)
+
+          Date.compare(today, cycle_end) in [:lt, :eq]
+
+        _ ->
+          false
+      end
+    end)
+  end
+
+  # Deletes future cycles and regenerates them with the new type/amount
+  defp delete_and_regenerate_cycles(cycles_to_delete, member_id) do
+    if Enum.empty?(cycles_to_delete) do
+      # No cycles to delete, just regenerate
+      regenerate_cycles(member_id)
+    else
+      case delete_cycles(cycles_to_delete) do
+        :ok -> regenerate_cycles(member_id)
+        {:error, reason} -> {:error, reason}
+      end
+    end
+  end
+
+  # Deletes cycles and returns :ok if all succeeded, {:error, reason} otherwise
+  defp delete_cycles(cycles_to_delete) do
+    delete_results =
+      Enum.map(cycles_to_delete, fn cycle ->
+        Ash.destroy(cycle)
+      end)
+
+    if Enum.any?(delete_results, &match?({:error, _}, &1)) do
+      {:error, :deletion_failed}
+    else
+      :ok
+    end
+  end
+
+  # Regenerates cycles with new type/amount
+  # CycleGenerator uses its own transaction with advisory lock
+  defp regenerate_cycles(member_id) do
+    case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member_id) do
+      {:ok, _cycles} -> :ok
+      {:error, reason} -> {:error, reason}
+    end
+  end
+
   # Normalizes visibility config map keys from strings to atoms.
   # JSONB in PostgreSQL converts atom keys to string keys when storing.
   defp normalize_visibility_config(config) when is_map(config) do

lib/membership_fees/changes/validate_same_interval.ex

@@ -37,29 +37,35 @@ defmodule Mv.MembershipFees.Changes.ValidateSameInterval do
     current_type_id = get_current_type_id(changeset)
     new_type_id = get_new_type_id(changeset)
 
-    # If no current type, allow any change (first assignment)
-    if is_nil(current_type_id) do
-      changeset
-    else
-      # If new type is nil, that's allowed (removing type)
-      if is_nil(new_type_id) do
+    cond do
+      # If no current type, allow any change (first assignment)
+      is_nil(current_type_id) ->
         changeset
-      else
-        # Both types exist - validate intervals match
-        case get_intervals(current_type_id, new_type_id) do
-          {:ok, current_interval, new_interval} ->
-            if current_interval == new_interval do
-              changeset
-            else
-              add_interval_mismatch_error(changeset, current_interval, new_interval)
-            end
 
-          {:error, _reason} ->
-            # If we can't load the types, allow the change (fail open)
-            # The database constraint will catch invalid foreign keys
-            changeset
+      # If new type is nil, that's allowed (removing type)
+      is_nil(new_type_id) ->
+        changeset
+
+      # Both types exist - validate intervals match
+      true ->
+        validate_intervals_match(changeset, current_type_id, new_type_id)
+    end
+  end
+
+  # Validates that intervals match when both types exist
+  defp validate_intervals_match(changeset, current_type_id, new_type_id) do
+    case get_intervals(current_type_id, new_type_id) do
+      {:ok, current_interval, new_interval} ->
+        if current_interval == new_interval do
+          changeset
+        else
+          add_interval_mismatch_error(changeset, current_interval, new_interval)
         end
-      end
+
+      {:error, _reason} ->
+        # If we can't load the types, allow the change (fail open)
+        # The database constraint will catch invalid foreign keys
+        changeset
     end
   end
 

test/membership_fees/changes/validate_same_interval_test.exs

@@ -215,8 +215,7 @@ defmodule Mv.MembershipFees.Changes.ValidateSameIntervalTest do
     defp extract_error_message(%Ash.Error.Invalid{errors: errors}) do
       errors
       |> Enum.filter(&(&1.field == :membership_fee_type_id))
-      |> Enum.map(& &1.message)
-      |> Enum.join(" ")
+      |> Enum.map_join(" ", & &1.message)
     end
   end
 end