The mechanical usage of the struct seems correct, but I'm having some conceptual issues with the original code.
First, the Stack
structure models a single element instead of the entire structure. I find this weird, and it makes it impossible to implement custom protocols for the stack abstraction. Your module seems to partially abstract stack elements, and partially the entire stack structure.
Second, what is the purpose of key/value? Stack should work on arbitrary elements. What is in those elements should be left to the client of the structure, and not hardcoded as a requirement of theStack
module.
Then, function push/3
accepts the abstraction as the last argument. This is contrary to the recommended conventions (subject as the first argument), and makes it impossible to use the abstraction with pipe operator |>
.
Even if the last issue was fixed, pipes still won't work since push
returns the result in format of {:ok, ...}
. This is not needed, especially since there is no possible error outcome.
Notice that this doesn't hold for pop
, which must return two values: last element pushed, and the modified structure (a stack containing remaining elements).
Disregarding the fact that stack really doesn't require an abstraction (Elixir list is already a stack), here's my take on it:
defmodule Stack do
defstruct elements: []
def new, do: %Stack{}
def push(stack, element) do
%Stack{stack | elements: [element | stack.elements]}
end
def pop(%Stack{elements: []}), do: raise("Stack is empty!")
def pop(%Stack{elements: [top | rest]}) do
{top, %Stack{elements: rest}}
end
def depth(%Stack{elements: elements}), do: length(elements)
end
This can then be used as:
iex(1)> Stack.new |> Stack.push(1) |> Stack.push(2) |> Stack.pop
{2, %Stack{elements: [1]}}