From c887e52faf8bbe50d81ef3f34e287b7ee4c616ec Mon Sep 17 00:00:00 2001
From: Jesse Van Volkinburg <vanvoljg@gmail.com>
Date: Wed, 29 Jul 2020 22:21:42 -0700
Subject: [PATCH] code review changes - xebow.ex

Changes state into a struct and improves the config update function
---
 lib/xebow.ex                      | 124 +++++++++++++++++++++---------
 lib/xebow_web/live/matrix_live.ex |   2 +-
 2 files changed, 90 insertions(+), 36 deletions(-)

diff --git a/lib/xebow.ex b/lib/xebow.ex
index 6e514a3..3613535 100644
--- a/lib/xebow.ex
+++ b/lib/xebow.ex
@@ -42,18 +42,31 @@ defmodule Xebow do
   @spec layout() :: Layout.t()
   def layout, do: @layout
 
+  @type animations :: [Animation.t()]
+  @type animation_params :: %{String.t() => atom | number | String.t()}
+
+  defmodule State do
+    @moduledoc false
+    defstruct [:current_animation, :next_list, :previous_list]
+  end
+
   use GenServer
 
   # Client Implementations:
 
+  @doc """
+  Starts the Xebow application, which manages initialization of animations, as well
+  as switching between active animations. It also maintains animation config state
+  and persists it in memory between animation changes.
+  """
   @spec start_link([]) :: GenServer.on_start()
   def start_link([]) do
     GenServer.start_link(__MODULE__, Animation.types(), name: __MODULE__)
   end
 
   @doc """
-  Gets the current animation configuration. This retrievs current values, which
-  allows for changes to be made with `update_animation_config/2`
+  Gets the animation configuration. This retrievs current values, which allows for
+  changes to be made with `update_animation_config/1`
   """
   @spec get_animation_config() :: {Animation.Config.t(), keyword(Animation.Config.t())}
   def get_animation_config do
@@ -77,11 +90,12 @@ defmodule Xebow do
   end
 
   @doc """
-  Updates the animation configuration for the current animation
+  Updates the configuration for the current animation
   """
-  @spec update_animation_config(Animation.t()) :: :ok | :error
-  def update_animation_config(animation_with_config) do
-    GenServer.call(__MODULE__, {:update_animation_config, animation_with_config})
+  @spec update_animation_config(animation_params) ::
+          :ok
+  def update_animation_config(params) do
+    GenServer.cast(__MODULE__, {:update_animation_config, params})
   end
 
   # Server Implementations:
@@ -92,53 +106,93 @@ defmodule Xebow do
       types
       |> Enum.map(&initialize_animation/1)
 
-    [current | _] = active_animations
+    current = hd(active_animations)
     Engine.set_animation(current)
-    state = {active_animations, []}
 
-    {:ok, state}
-  end
+    state = %State{
+      current_animation: current,
+      next_list: active_animations,
+      previous_list: []
+    }
 
-  @impl GenServer
-  def handle_call(:get_animation_config, _caller, state) do
-    {[current | _rest], _previous} = state
-    {:reply, Animation.get_config(current), state}
+    {:ok, state}
   end
 
   @impl GenServer
-  def handle_call({:update_animation_config, animation_with_config}, _caller, state) do
-    {[_current | rest], previous} = state
-    {:reply, :ok, {[animation_with_config | rest], previous}}
+  def handle_call(:get_animation_config, _from, state) do
+    {:reply, Animation.get_config(state.current_animation), state}
   end
 
   @impl GenServer
   def handle_cast(:next_animation, state) do
-    case state do
-      {[current | []], previous} ->
-        remaining_next = Enum.reverse([current | previous])
-        Engine.set_animation(hd(remaining_next))
-        {:noreply, {remaining_next, []}}
-
-      {[current | remaining_next], previous} ->
-        Engine.set_animation(hd(remaining_next))
-        {:noreply, {remaining_next, [current | previous]}}
+    case state.next_list do
+      [] ->
+        [new_current | next_list] = Enum.reverse([state.current_animation | state.previous_list])
+        Engine.set_animation(new_current)
+
+        state = %{
+          state
+          | current_animation: new_current,
+            next_list: next_list,
+            previous_list: []
+        }
+
+        {:noreply, state}
+
+      [new_current | next_list] ->
+        Engine.set_animation(new_current)
+
+        state = %{
+          state
+          | current_animation: new_current,
+            next_list: next_list,
+            previous_list: [state.current_animation | state.previous_list]
+        }
+
+        {:noreply, state}
     end
   end
 
   @impl GenServer
   def handle_cast(:previous_animation, state) do
-    case state do
-      {remaining_next, []} ->
-        [next | remaining_previous] = Enum.reverse(remaining_next)
-        Engine.set_animation(next)
-        {:noreply, {[next], remaining_previous}}
-
-      {remaining_next, [next | remaining_previous]} ->
-        Engine.set_animation(next)
-        {:noreply, {[next | remaining_next], remaining_previous}}
+    case state.previous_list do
+      [] ->
+        [new_current | previous_list] = Enum.reverse([state.current_animation | state.next_list])
+        Engine.set_animation(new_current)
+
+        state = %{
+          state
+          | current_animation: new_current,
+            next_list: [],
+            previous_list: previous_list
+        }
+
+        {:noreply, state}
+
+      [new_current | previous_list] ->
+        Engine.set_animation(new_current)
+
+        state = %{
+          state
+          | current_animation: new_current,
+            next_list: [state.current_animation | state.next_list],
+            previous_list: previous_list
+        }
+
+        {:noreply, state}
     end
   end
 
+  @impl GenServer
+  def handle_cast({:update_animation_config, params}, state) do
+    updated_animation = Animation.update_config(state.current_animation, params)
+    Engine.set_animation(updated_animation)
+
+    state = %{state | current_animation: updated_animation}
+
+    {:noreply, state}
+  end
+
   defp initialize_animation(animation_type) do
     Animation.new(animation_type, @leds)
   end
diff --git a/lib/xebow_web/live/matrix_live.ex b/lib/xebow_web/live/matrix_live.ex
index bd92070..0e31ca1 100644
--- a/lib/xebow_web/live/matrix_live.ex
+++ b/lib/xebow_web/live/matrix_live.ex
@@ -63,7 +63,7 @@ defmodule XebowWeb.MatrixLive do
   def handle_event("update_config", %{"_target" => [field_str]} = params, socket) do
     value = Map.fetch!(params, field_str)
 
-    Engine.update_animation_config(%{field_str => value})
+    Xebow.update_animation_config(%{field_str => value})
 
     {:noreply, socket}
   end