How would PragDave separate this code?

Fl4m3Ph03n1x

Background

After reading Elixir in Action I came with a (slightly modified for my evil purposes) module that I very originally call the ProcessRegistry. Now this module works and is fine, it also has a behaviour defined, but everything is in the same file and it feels tangled to me, specially after reading this post from pragmatic Dave Splitting APIs, Servers, and Implementations in Elixir.

Code

So, following is the file called ProcessRegistry:


defmodule Clint.IProcessRegistry do
  @type via_tuple_type  ::
    {:via, module, {module, {module, any}}} |
    {:via, module, {module, {module, any}, any}}

  @callback lookup(tuple)                  :: [{pid, any}]
  @callback update_with_value(tuple, any)  :: {any, any} | :error
  @callback via_tuple(tuple)               :: via_tuple_type
  @callback via_tuple(tuple, any)          :: via_tuple_type
end

defmodule Clint.ProcessRegistry do
  @behaviour Clint.IProcessRegistry

  alias Clint.IProcessRegistry

  ###############
  # Public API  #
  ###############

  @impl IProcessRegistry
  def lookup(key), do: Registry.lookup(__MODULE__, key)

  @impl IProcessRegistry
  def update_with_value(key, new_value), do:
    Registry.update_value(__MODULE__, key, fn _old_value -> new_value end )

  @impl IProcessRegistry
  def via_tuple(key), do:
    {:via, Registry, {__MODULE__, key}}

  @impl IProcessRegistry
  def via_tuple(key, value), do:
    {:via, Registry, {__MODULE__, key, value}}

  @spec start_link() :: {:ok, pid} | {:error, any}
  def start_link, do:
    Registry.start_link(keys: :unique, name: __MODULE__)

  #############
  # Callbacks #
  #############

  @spec child_spec(any) :: Supervisor.child_spec
  def child_spec(_) do
    Supervisor.child_spec(
      Registry,
      id: __MODULE__,
      start: {__MODULE__, :start_link, []}
    )
  end
end

It’s a fairly small file with 53 lines, but it has a couple of things that bother me:

The Contract and the Implementation are defined in the same file. I really don’t like this.
According to pragmatic Dave, the API and the implementation are mixed together.

For the first point, I don’t really think there will ever be another implementation of ProcessRegistry that doesn’t use Elixir’s Registry. Yes, I could go on an adventure and re-invent the wheel, but what for?

How many of you had to re-implement registry’s functionality for your own projects after Elixir 1.4? What is so bad about the current implementation of Registry (or so lacking) that would make you re-implement it? Short summary, I don’t really think I need a Contract for this, because I don’t see a future where ProcessRegistry will have (or need) an implementation that doesn’t use Elixir’s Registry.

The second point is even more contentious. From what I understand, even without the behaviour, Dave would not approve of my “mixing of concerns” between the API and the Registry module. He would define a module called ProcessRegistry with the API, and then inside a folder with the same name he would create a module with a completely useless name like impl and there I would have the calls to Registry.

I attack this idea based on the fact Dave likes to complain about useless folders and boilerplaty code, but I find that in my specific case, this is what I would be creating - more boilerplate and indirection.

Opinions

Have you read Dave’s small article? What do you think?
How can I improve this module and this code? Let me know !

26 comments

#api #registry

1 3706 26

2019-05-10 14:40:13 UTC

Showing Posts 1 to 10

Show Best Posts
Show All (oldest first)
Show All (newest first)

peerreynders

Previous discussions:

Post #1

Fl4m3Ph03n1x

I have read through both those two discussions before, but I don’t think they pinpoint the issues I bring in this post. This is not about modularization of applications by dividing them in many many small components that you can then link together via a config file, this is about separation of concerns between the API and its implementation and is something I didn’t see discussed thus far.

Even if it was, what I am looking for is some feedback specific to this sample. So, @peerreynders, how would you do it? This is what I want to know - you and the why of you

Post #2

jeremyjh

Dave’s opinions here are at odds with established norms in Erlang and Elixir. I’m not trying to rehash the arguments just pointing out that Dave is exploring new territory and proposing things that a lot of people don’t agree with. I’m curious though if you’ve examined your ideas about why you do not like it? I like it because it completely encapsulates the message-passing interface, which is no one else’s business.

They don’t have to be. You could have a public behaviour for your module that defines the interface separately.

I think a registry is provided simply as an example of how these different pieces fit together. I’ve never implemented a process registry.

Post #3

LostKobrakai

If you just want to start the registry and nothing more you can just do:

  @spec child_spec(any) :: Supervisor.child_spec
  def child_spec(_), do: Registry.child_spec(keys: :unique, name: __MODULE__)

No need for a more complex child_spec.

Post #4

LostKobrakai

A behaviour is still useful even if there’s only one implementation. It’s not only a contract which makes switching out implementations easier, but it’s also a contract between caller and implementation, which is more explicit than “oh there’s a public function on the implementation module”. So if you already have a useful behaviour keep it around.

For the one file issue: Just put them in two files and the issue is gone.

The “indirection” of behaviours you mentioned is not in the fact that you have a behaviour, but you get indirection only if you no longer call MyOnlyImpl.via_tuple(key), but you need a more flexible system, where switching out the actual implementation is possible. That part you can skip if you don’t need it. As long as the caller only uses callbacks defined in the behaviour you can add a more flexible system later at any time.

Your module hardly does stuff anyways, so spliting it up is surely overkill. Even if you like the ideas of Dave you’ll always need to make the decision if the tradeoff of more files is worth the greater abstraction. Creating more stuff for the sake of having more stuff is never a good idea. To turn the idea around. If you build a genserver and each callback has multiple hundred lines of state modification, task spawning and return value calculation in it you might be in a place where spliting up state modification, runtime message passing and worker spawning into concrete modules will be way more useful.

In my opinion “separation of concern” is a bad metric. What even is a concern? E.g. in my mind your example module is a single concern, because it bundles stuff needed to interact with a registry. Nobody else needs to know details about it but this module. You seem to not trust your instinct in that it actually is one concern.

Post #5

peerreynders

“It depends”.

For the case you are presenting a single file works.

But I also have to point out that Dave’s KV “API” isn’t a behaviour module. It simply is a module that represents an API.

You chose to define your “interface” formally via a behaviour module.

The reasons for these choices aren’t based on the same motivations and therefore not really related or comparable. In some ways the question seems to conflate two different viewpoints on APIs.

Dave’s approach

kv.ex
kv/impl.ex

kv.ex        ... API (not a behaviour module)
kv/impl.ex   ... data structure module
kv/server.ex ... server

is about speculative generality - “Now I have a data structure that later I may want to turn into a server”.

I assume Clint.IProcessRegistry exists to enable mock-ability.

And more importantly, while a behaviour module can be used to mimic an interface/contract - that isn’t its raison d’etre.

Behaviours were designed for composition.

The behaviour module (e.g. gen_server) encapsulates generic capability - i.e. it includes code.
The callback module provides specialized capability (more code) to specialize run-time behaviour.
The contract between both has to exist in order for the composition to work.

Post #6

Rich_Morin

I’m using Dave’s approach in my current project. I really like splitting the API apart from the implementation and the server apart from the code that produces the served data.

Even my largest modules are only about 300 LOC, which I find quite manageable. FWIW, here is the lib directory for one of the smaller apps.

I’m using an umbrella app structure, because Mix works well with this, but I’d be happy to switch over to a component-style approach if and when it is supported by production tooling.

Incidentally, I’d love to find a way to tidy up the @doc attributes for the defdelegate entries. As it is, I have to hand-edit repetitive details to produce a link to the referenced implementation function:

 @doc """
  Return a Map describing the code files.
  ([`...CntCode.get_code_info/1`] (InfoFiles.CntCode.html#get_code_info/1))
  """
  defdelegate get_code_info(tree_base),             to: CntCode

Post #7

yordisprieto

I really like to have the GenServer and the implementation separated from each other.

Client
ClientState (just handling the mutation of the data structure from the client)
Server(GenServer stuff)

Probably I will follow this (I may no need ClientState sometimes) for most projects.

Post #8

sasajuric

Author of Elixir In Action

I don’t really understand the purpose of this module. In a real project, I’d likely delete this code and use Registry directly.

If you really insist on having this module, the second best simplification would IMO be to remove the IProcessRegistry contract, and use ProcessRegistry directly.

I definitely wouldn’t split this particular code into more files. That would just lead to more indirections which would needlessly degrade the reading experience.

Post #9