Usage of {:ok, result} / :error vs {:some, result} / :none

[OvermindDL1]

@jeffdeville

I’m playing with Exceptional. I do like the idea of using exception structs as return values, because structs aren’t positional the way tuples are. Definitely that feels like a better answer. However, I haven’t found a case where I feel it improves on the with/do/else syntax. I tried to recreate my earlier example with it, and it came out the same, aside from my ‘else’ block matching maps instead of tuples. So far, I’m just not sure it’s worth breaking with idiomatic elixir for that.

@OvermindDL1 - I’m not a classically trained functional programmer. Am I missing something?

The exceptional library and the returning-exception format was designed for easy piping and great compatibility with Plug/Phoenix (remember that your custom exception can give its own error and status code with Plug/Phoenix, making it nice to propagate them up). On its own it is not really much better or worse then, say, tagged tuples (well, it is better in Phoenix regardless), but piping tagged tuples is far more difficult (especially considering the myriad of formats). Your above example, replicated here:

@jeffdeville

with(
      client_ip = get_client_ip(request_ip, params),
      {:ok, location} <- Geoip.lookup_ip(client_ip),
      true <- should_search?(location, parse_exclusion_zone(params)),
      query = build_search_query(%SearchQuery{}, location, params),
      {:ok, results} <- NHSearch.search(query),
      Analytics.track(results, client_ip, "api", "search_results")
) do
      json conn, results
else
  {:error, :location_not_found} ->
    Logger.error("Unable to find the location for: #{get_client_ip(request_ip, params)}")
    # Need to make this an error message of some sort to explain what's going on.
    json conn, []
  :user_in_exclusion_zone ->
    Logger.info("User is too close to the exclusion box")
    # Need to make this an error message of some sort to explain what's going on.
    json conn, []
  other ->
    Logger.error(inspect(other))
    json conn, []
end

If done in the exceptional ‘style’ (with full importing, which is not default, but I like it), while not changing the exposed API (which if you did would make it even more clean), would become something like this:

request_ip
|> get_client_ip(params)
|> Geoip.lookup_ip() |> normalize # normalize takes a tagged-tuple and converts it to an ErlangError or unwraps an :ok tuple
|> bool_test(should_search?/1).(parse_exclusion_zone(params)) # This is proposed in a PR right now to wrap true/false tests, passes original value if true, else returns exception
|> (&build_search_query(%SearchQuery{}, &1, params)).() # Why does your build_search_query take `location` as its second instead of first arg?
|> NGSearch.search() |> normalize
|> Analytics.track(client_ip, "api", "search_results")
|> (&json(conn, &1)).()
|> ensure! # This means if any exception was returned at any prior point, then raise it instead

And since I raised the exception at the end if any existed then the exceptions are handled as normal via phoenix, and you can decorate them and return them with status codes and add handlers in your error handler to handle errors properly (which ‘that’ is where your json error handling should be, not in your controller in my opinion), and of course they will get logged as normal exceptions do, etc…

The API could of course be cleaned up a lot by re-orienting arguments and such:

request_ip
|> get_client_ip(params)
|> Geoip.lookup_ip() |> normalize # normalize takes a tagged-tuple and converts it to an ErlangError or unwraps an :ok tuple
|> should_search(parse_exclusion_zone(params))
|> build_search_query(%SearchQuery{}, params)
|> NGSearch.search() |> normalize
|> Analytics.track(client_ip, "api", "search_results")
|> (&json(conn, &1)).()
|> ensure!

Or something like that, which can always be cleaned up further too (and using other libraries like tap or so you can put arguments in non-first positions in a pipe-chain as well).

Using normal use Exceptional without other options, the above last chain would look like:

request_ip
|> get_client_ip(params)
|> Geoip.lookup_ip() |> normalize # normalize takes a tagged-tuple and converts it to an ErlangError or unwraps an :ok tuple
~> should_search(parse_exclusion_zone(params))
~> build_search_query(%SearchQuery{}, params)
~> NGSearch.search() |> normalize
~> Analytics.track(client_ip, "api", "search_results")
~> (&json(conn, &1)).()
|> ensure!

[OvermindDL1]

@rvirding

This case is often forgotten which can complicate its usage.

Entirely, and if you make your own exceptions in things, then put something like plug_status: 400 in it to auto-set the proper status codes for Plug/Phoenix.

Exceptional’s version of matching on tagged tuples in the case version is what it calls if_exception, the do is what to do with an exception struct, the else is the success side (or pass it on).

[michalmuskala]

I don’t agree that always piping everything is a good way. While it might be very easy to write, I find the given examples nearly completely illegible. They are very “dense” in behaviour and many different things are occurring at once. Treating different things, differently is not always a bad thing.

Going back to the original question about {:some, value} | :none, I’m not sure I see any difference with the currently used {:ok, value} | :error - it behaves exactly the same in all occurrences. The only difference is the names, but I’m not sure it’s worth going against the established convention in the entire platform (both Erlang and Elixir), just to have a slightly better name.

[jeffdeville]

Ha! I’m just gonna keep replying until you’ve divulged all of your cool helper libraries, @OvermindDL1

I assume ‘tap’ was this:

mgwidmann/elixir-pattern_tap

That said, I’m not sold on quite all of this yet. Specifically:

• The opaqueness of the exception handling concerns me. It takes for granted that exceptions thrown will be obvious enough that what should be done with them will be consistent and self-evident from lower in the stack than your controllers. This skeeves me a bit. It violates the law of demeter, and feels like it’s skirting a little too close to the realm of rails-ish magic, for my taste. I’ve seen how that can be handy with things like 404 errors in phoenix, but I can’t help but feel that an exception handling plug would have been preferable.
• Converting a boolean test into an exception (bool_test(should_search?/1)) feels like too much, because I’d HAVE to use try/catch, and then rescue an obscure ErlangError(false) or something many lines down in order to handle it properly.
• I have to agree that sometimes piping gets overwhelming. What I think makes me feel that way is when I start returning entirely different values from 1 pipe to another. In unix, you cat a file (text) through grep to get (text) and then sed to get more text. That’s all fine, because the data you’re working on, while changing, is still consistent. So while your refactoring certainly looks beautiful, the data being operated upon varies wildly, and I feel like that might be what @michalmuskala was alluding to. (clientip -> location -> search query -> search results)

All that said, this is an awesome thread. I’ve learned a lot this afternoon, and been given much to consider and play with.

[Crowdhailer]

I try as often as possible to use the combination of {:ok, value} and {:error, reason}. Thats because this particular combination can be thought of as an analogue to a result tuple and that has some nice properties. I have written a library that takes advantage of restricting myself to only those to forms so it allows piping possible results. I normally pair this with a final pipe to handle the error cases. As in the example below

import OK, only: :macros

def get_employee_data(file, name) do
  {:ok, file}
  ~>> File.read
  ~>> Poison.decode
end

def handle_user_data({:ok, data}), do: IO.puts("Contact at #{data["email"]}")
def handle_user_data({:error, :enoent}), do: IO.puts("File not found")
def handle_user_data({:error, {:invalid, _}}), do: IO.puts("Invalid JSON")

get_employee_data("my_company/employees.json")
|> handle_user_data

I have found it easy to limit myself to only result tuples.

• Don’t use :error always add a reason. e.g. {:error, :not_found}
• Don’t return just :ok, If there really is no return value {:ok, nil} is fine to me. I try to reduce the number of side effect casing functions so I have very few of these. Other wise I return one of the arguments as a value so I can pipe to subsequent actions.

[OvermindDL1]

@michalmuskala

I don’t agree that always piping everything is a good way. While it might be very easy to write, I find the given examples nearly completely illegible. They are very “dense” in behaviour and many different things are occurring at once. Treating different things, differently is not always a bad thing.

They are yes, but the original example was dense, I’d have broken it up into more functions (I’m a fan of the max-of-2-function-calls-per-function style in general). ^.^

@jeffdeville

I assume ‘tap’ was this:

No, that one matches out back in to a pipe, exceptional can do that and more, I’m talking about pipe_here but I had the wrong name. ^.^

@jeffdeville

Converting a boolean test into an exception (bool_test(should_search?/1)) feels like too much, because I’d HAVE to use try/catch, and then rescue an obscure ErlangError(false) or something many lines down in order to handle it properly.

I already do this myself, but via using the happy library (which is older than the current with and more powerful, but otherwise similar), I have things like this littered about my code:

      @perm true = conn |> can?(show(%Perms.CheckInOut{section: true}))
      {:ok, section} = verify_single_record get_section_by_slug(slug_param)
      @perm true = conn |> can?(show(%Perms.CheckInOut{section: section.slug}))

The first line is a permission test to see if they have access to any sections at all, the second line is it gets a record (and dies if verify_single_record fails), and the third line verifies they have access to this specific section, it is more declarative, but it can be done via piping too, however I do like names. ^.^

I do agree with the obscure ErlangError though, however I have my tests return a success or fail, not just true/false (I’m not a fan of true/false as it does not give better information, I think permissions is the only area where that is done at all, and that is because can requires it). ^.^