Code review: an IPv4 module

Hi, I’d appreciate any feedback about the coding style, and way this works. I wrote it this way to “make an invalid value unrepresentable”. Thanks for any feedback!

defmodule IPv4 do
  @enforce_keys [:octets]
  defstruct [:octets]

  defguard is_octet(number) when is_integer(number) and number >= 0 and number <= 255
  defguard are_octets(a, b, c, d) when is_octet(a) and is_octet(b) and is_octet(c) and is_octet(d)

  def new_from_string(ip) when is_bitstring(ip) do
    [a, b, c, d] = String.split(ip, ".") |> Enum.map(&String.to_integer/1)
    new(a, b, c, d)
  end

  def new(a, b, c, d) when are_octets(a, b, c, d) do
    %IPv4{octets: {a, b, c, d}}
  end

  def ptr_domain(%IPv4{octets: {a, b, c, d}}) do
    reverse_addr_string = %IPv4{octets: {a, b, c, d}} |> reverse |> IPv4.to_string()
    "#{reverse_addr_string}.in-addr.arpa"
  end

  def reverse(%IPv4{octets: {a, b, c, d}}), do: %IPv4{octets: {d, c, b, a}}

  def to_string(%IPv4{octets: {a, b, c, d}}), do: "#{a}.#{b}.#{c}.#{d}"
end

I would rewrite it like so:

defmodule IPv4 do
  @enforce_keys [:octets]
  defstruct [:octets]

  defguard is_octet(number) when is_integer(number) and number >= 0 and number <= 255
  defguard are_octets(a, b, c, d) when is_octet(a) and is_octet(b) and is_octet(c) and is_octet(d)

  def new_from_string(ip) when is_bitstring(ip) do
    [a, b, c, d] =
      ip
      |> String.split(".")
      |> Enum.map(&String.to_integer/1)

    new(a, b, c, d)
  end

  def new(a, b, c, d) when are_octets(a, b, c, d) do
    %__MODULE__{octets: {a, b, c, d}}
  end

  def ptr_domain(%{octets: {a, b, c, d}} = ip) when are_octets(a, b, c, d) do
    ip
    |> __MODULE__.reverse()
    |> __MODULE__.to_string()
    |> Kernel.<>(".in-addr.arpa")
  end

  def reverse(%{octets: {a, b, c, d}}) when are_octets(a, b, c, d), do: new(d, c, b, a)

  def to_string(%{octets: {a, b, c, d}}) when are_octets(a, b, c, d), do: "#{a}.#{b}.#{c}.#{d}"
end

Changes:

  1. Stricter pipe syntax (always have the starting variable be alone at the start).
  2. Make the local function calls to be neutral to the name of the module they currently reside in (replaced IPv4 with __MODULE__).
  3. Using Kernel.<> to avoid having a local variable.
  4. ptr_domain, reverse and to_string accept not only the struct itself but also a map with the proper structure (which conforms to the guard as well);
  5. Use the are_octets guard in ptr_domain, reverse and to_string in case you are accepting an arbitrary map and not the struct.
  6. Use new in reverse.

4 and 5 are obviously optional but I’d still do them because often times you have well-shaped parameter maps and you can reuse the same functions on them (especially if you use Absinthe and are not in a Phoenix controller which always has string map keys; in contrast, Absinthe gives you atom-keyed maps IIRC).

2 Likes

I believe the is_bitstring check is overly loose: def new_from_string(ip) when is_bitstring(ip) do. If you’re expecting a string like "255.255.255.255" then you’d expect a binary and check it with is_binary/1 instead. Bitstrings are for binaries which don’t fall into the 8 byte boundary.

iex(1)> is_bitstring("1.2.3.4")
true
iex(2)> is_bitstring(<<1::size(4)>>)
true
iex(3)> is_binary("1.2.3.4")
true
iex(4)> is_binary(<<1::size(4)>>)
false
2 Likes

Interesting - I didn’t know that - thanks!

Thanks! This is a lot to think about. #2 makes sense, but it’s uglier :stuck_out_tongue: It’s DRY’er though.

#1, #3, and #6 I find completely uncontroversial.

So #4 and #5 are interesting: essentially, being more accepting about inputs, yet still checking for type.

Yep. Consider this simplified example:

defmodule Account do
  defstruct [:name, :balance]

  def consume_record(%{name: name, balance: balance})
  when is_binary(name)
  and  is_integer(balance) do
    IO.puts "#{name} has #{balance} euro-cents."
  end

  def test() do
    %Account{name: "John", balance: 5177}
    |> consume_record()

    %{name: "Sve", balance: 19806}
    |> consume_record()
  end
end

Invoking Account.test() in iex gives you this:

iex(1)> Account.test
John has 5177 euro-cents.
Sve has 19806 euro-cents.
:ok

So yep, as long as you deconstruct an input you can assert types and even value structure on it.

1 Like