How Does This Naive Stack Implementation Look?

Consider this naive stack implementation in Elixir:

defmodule Stack do
  defstruct name: "", value: 0

  def init() do
    _s = []
  end

  def push(name, value, s) do
    s_new =  [%Stack{name: name, value: value}, s]
    {:ok,s_new}
  end

  def pop(s) do
    [h|tail] = s
    {:ok, h, tail}
  end

  def depth(s) do
    length(s)
  end

end

#Use:
# s = Stack.init()
# {:ok, s} = Stack.push("a",1,s)
# {:ok, item, s} = Stack.pop(s)
# l = Stack.depth(s)

Am I using structs correctly? Any suggestions for more idiomatic code style?

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]}}

StackExchange Code Review Q#48733, answer score: 5