Discussion: Complex API error handling

[josevalim]

Not sure if the issues tracker is the correct place to discuss it but I don't necessarily agree with this smell. Semantically speaking, both solutions are equivalent. Multiple function clauses are actually compiled to a case. Second, the multiple function version is quite more verbose and indirect, which I think makes reading the code harder. I think it is more preferred to go from multiple clauses to a case (perhaps to address "Complex multi-clause function"), than vice-versa. :)

[josevalim]

I have also opened #9, which is somewhat related to this discussion. Btw, a possible fix for "Complex multi-clause function" is to provide specific functions, such as update_product, update_animal, etc. :)

[keathley]

I also don't agree with this smell. The original is direct. Based on my experience with this pattern, the "fix" is harder to maintain in the long run.

[lucasvegi]

This discussion is really interesting and I understand your point of view. Anyway, I would like to raise other questions so that we can discuss this.

In the example presented in the catalog, get_customer/1 is a relatively small function, so by breaking it into several others, we actually have more verbose code.

But let's imagine that get_customer/1 was a really long function, responsible for handling a much larger number of return types from an endpoint. Under these circumstances, I think this function could get very confusing, difficult to maintain, and bug-prone (like almost any long function). In addition, because it concentrates all the return handling of an endpoint, a typo, or any problem introduced by the programmer in handling a return type, it could eventually compromise the handling of all returns from the endpoint (if the function raises an exception, for example).

In this more complex scenario, I imagine that breaking this function into several others is less risky. What do you think (@josevalim and @keathley)?

[josevalim]

Your points are valid. I would add two things:

• I would say part of the issue you describe is rather “long functions” or “branching complexity”

• the issue with types and complex return types still happen with multiple function clauses, so the refactoring is not addressing it

For example, I could still have a case but each branch delegates to a different private function. In this scenario, the code would be cleaner, perhaps even more concise than the refactoring, while keeping the case.

I was reading the short paper and I noticed we don’t have the “classical code smells” of “long functions” and “long parameter lists” listed. Perhaps we should convert this one to “long functions”?

[lucasvegi]

Hi @josevalim ,

your observations are very good. I liked your suggestion of refactoring removing the pattern matching and keeping the case. In this situation, I also think the code will be cleaner and more readable. I implemented a code version of this refactoring but would like to know your opinion before updating the catalog. Does your refactoring suggestion follow the following idea?

def get_customer(customer_id) when is_integer(customer_id) do
  case get("/customers/#{customer_id}") do
    {:ok, %Tesla.Env{status: 200, body: body}} -> success_api_response(body)
    {:ok, %Tesla.Env{body: body}} -> x_error_api_response(body)
    {:error, _} = other -> y_error_api_response(other)
  end
end

defp success_api_response(body) do
  {:ok, body}
end

defp x_error_api_response(body) do
  {:error, body}
end

defp y_error_api_response(other) do
  other
end

In addition to updating the refactoring code example, I intend to bring more details into the problem or example description of what we are discussing here. I think it's important to make it clear to the reader that get_customer/1 is not really long, but it could be... So the ideal is for the reader to abstract this possibility, thinking about a more complex scenario, where a large number of different returns could be provided through the same endpoint.

About the paper, in table 1 we list 11 traditional smells that were being discussed in some way in the grey literature. Among them, we do have "Long function" and "Long Parameter List". We think it's better to keep "Complex API error handling" as a new smell as it is a very specific type of "Long function" and has implications of its own. Anyway, I think it's worth mentioning more clearly in the description of this smell that it is a "specialization" of "Long Function".

The reason we choose to maintain this smell as new is: If we overgeneralize the specific problems in a single smell, we run the risk of it becoming a "Complex smell"... with that we get into the same situation that we are discussing here.. :)

This type of decision needed to be made in other parts of our research as well, and in some cases, the choice was to generalize. In the paper, we highlight the case of Shotgun Surgery. Although we found a discussion in a very specific scenario (microservices), there were no implications of their own, so we generalized the problem.

This type of discussion that we are having is very important for the validation of our research. In the next step of our work, we intend to carry out a survey (interviews and questionnaires) with a large number of Elixir developers. The objective will be to understand their perceptions of each smell (Prevalence and Severity). We are just waiting for the ethics committee to approve the procedures we intend to use for this validation.

We are excited to get to this part of the research! This will make our results more robust.

[josevalim]

Yes, the updated code example is perfect. The only thing I would change then is the name of the smell... Perhaps complex branching? The issue and the following solution feel a bit related to cyclomatic complexity too.

This is also related to #9, as mentioned before. :)

[josevalim]

The nice thing about making this about branching is that it would also apply to cond, receive and other similar constructs. :)

[lucasvegi]

Hi @josevalim ,

I've updated the smell documentation a lot to incorporate snippets from our discussion. Also, I updated the refactoring example and renamed the smell (the new name is much better!).

What do you think (2c7c1a1)?

[josevalim]

Looks great!!!