rogach

rogach

I often need to abstract some common functionality into a separate module. In cases when this functionality requires keeping some state, I use the following pattern: I keep that state under a separate key in the “parent” module state (usually a GenServer), and to simplify the code I pass “parent” state into the functions of common module.

I’ll give a specific example to make the idea clearer. In the example, there is a GenServer module that handles packets coming from a TCP socket, and there is LineReader module that handles aggregating the raw data packets and extracting lines from them.

Here’s the code (I’ve attempted to omit most of irrelevant parts):

defmodule ClientHandler do
  use GenServer

  def init(%{socket: socket}) do
    state = %{
      socket: socket,
      lr_state: LineReader.initial_state(),
    }
    {:ok, state}
  end

  def handle_info({:tcp, _socket, data}, state) do
    state = LineReader.put_packet(state, data)
    {lines, state} = LineReader.recv_lines(state)
    # ... do something with lines
    {:noreply, state}
  end
end

defmodule LineReader do
  def initial_state() do
    %{lines: [], trailing: ""}
  end

  def put_packet(parent_state, packet) do
    lr_state = parent_state.lr_state
    # ... process the new data packet, update lr_state
    %{parent_state | lr_state: lr_state}
  end

  def recv_lines(parent_state) do
    lr_state = parent_state.lr_state
    lines = lr_state.lines
    lr_state = %{lr_state | lines: []}
    {lines, %{parent_state | lr_state: lr_state}}
  end
end

I feel like this code is a little smelly, because it ties into specific field name in parent state. However, it saves a lot of code on the caller’s side and makes the code much cleaner.

Is such approach an anti-pattern? Are there some better alternatives?

Showing Posts 1 to 10

LostKobrakai

LostKobrakai

It’s a little unclear why LineReader would need access to parent_state. I’d argue that LineReader should only be involved with the state it takes care of and not care that it’s nested within the ClientHandler. Yes this might be a “bit more characters” in ClientHandler, but it’ll be less in LineReader and responsibilities are properly aligned.

rogach

rogach OP

It’s a little unclear why LineReader would need access to parent_state. I’d argue that LineReader should only be involved with the state it takes care of and not care that it’s nested within the ClientHandler. Yes this might be a “bit more characters” in ClientHandler, but it’ll be less in LineReader and responsibilities are properly aligned.

That’s probably the part I’m most unsure about. I’ve chosen that approach mostly because of readability, and in addition because of less possibility for an error.

Compare:

{lines, state} = LineReader.recv_lines(state)

with:

{lines, lr_state} = LineReader.recv_lines(state.lr_state)
state = %{state | lr_state: lr_state}

While second version doesn’t need to access parent state, it adds quite a bit of ceremony and makes it easy to actually forget to update parent state field (immutable nature of Elixir might make it very easy to forget the second line and result in a bug).

rogach

rogach OP

and makes it easy to actually forget to update parent state field (immutable nature of Elixir might make it very easy to forget the second line and result in a bug).

I’m incorrect here - Elixir compiler will warn about an unused lr_state value, so it won’t be that easy to forget to update the parent state.

LostKobrakai

LostKobrakai

You could look into get_and_update_in(), update_in, ….

rogach

rogach OP

I don’t see how update_in or get_and_update_in will be useful here - we actually need to get the lines data out, and update_in only allows us to mutate contents of the map without returning anything.

LostKobrakai

LostKobrakai

get_and_update_in can return you a value and the update part could just be a noop if necessary.

rogach

rogach OP

I see now. So something like that will work:

{lines, state} = get_and_update_in(state.lr_state, &LineReader.recv_lines/1)
dimitarvp

dimitarvp

A super generic answer: be explicit. Implicitness is rarely worth it unless it’s seriously getting in the way. One example of “getting in the way”, subjectively for me, is a lot of artifacts that Phoenix generates should start off being referenced as defaults in other modules / libraries and only put inside your code base when you need to modify them and they are no longer the defaults.

In your case however, it does seem you want to save just a little typing which is IMO not worth.

And this is also very related to feature envy and the single responsibility principle. Simplified: put things where they semantically belong. Which of course is the entire problem, sometimes we have a problem nailing the semantics – and is ironically one of the things we are actually being paid to do, not the code flinging itself.

derek-zhou

derek-zhou

Smelly code is not clean code. My advise is to make the code not smelly first. We can then help you to refactor the code to be more concise and readable.

codeanpeace

codeanpeace

These might be signs that the LineReader would be better off as its own process i.e. Task/Agent/Genserver so it can keep track of its own state or rolled into ClientHandler. I’d be curious what else it’s being used for and how else it’s being used.

Where Next? Top

Trending in Questions Top

Blokh
Hey guys, I’ve got a huge CSV ( around 10 GB ) that needs to be processed hourly Do you guys have any suggestions what is the best prac...
New
kszambelanczyk
Hello! Could someone please give me a help/sample code, how to delete a file from s3 using waffle/waffle_ecto from Phoenix app. I creat...
New
Onor.io
I have what I’ve heard referred to as a “lookup table” in my database. This is a way of assigning codes to common values. One common lo...
New
Trolleger
What approach to take when sending live updates to “random” users Hi! I have a question, I have a little chat app, and when I create a DM...
New
RemyXRenard
I’m seeing that a list inside a Kino.DataTable will be interpreted as a charlist, even if the Kino.configure() is set to charlists: :as_l...
New
matt-savvy
Anyone here using Honeybadger? My Honeybadger account is being overwhelmed with noise from some bots. Seeing a lot of Bandit.HTTPError...
New
samoloth
Hi, I’ve just set up an application with ash_authentication. There is only magic link strategy for now, so there is no confirmation add o...
New

Other Trending Topics Top

mudasobwa
I am happy to introduce the very α version of the new programming language compiled to BEAM. Welcome Cure. It has literally three kille...
New
garrison
Hobbes is a low-level distributed database for the Elixir programming language. Hobbes provides a simple, safe, and scalable storage lay...
New
mcass19
ExRatatui lets you cook up rich terminal UIs in Elixir, powered by Rust’s ratatui via Rustler NIFs. Build interactive terminal applicatio...
New
Damirados
Hello everyone. After busy few months I am happy to announce v0.1.0 of Emerge & Solve. They are GUI (Emerge) and State management (S...
New
netoum
Corex is an accessible, unstyled UI component library for Phoenix that integrates Zag.js state machines using Vanilla JavaScript and Live...
New
wintermeyer
There are three potential reasons for members of this forum to have a look at https://vutuv.de You are tired or annoyed of LinkedIn. Yo...
New

We're in Beta

About us Mission Statement

Options

Thread Display Mode




Thread Preview

Skip Thread Previews