Skip to content

Conversation

@lucasvegi
Copy link
Contributor

No description provided.

lucasvegi and others added 30 commits July 14, 2023 09:44
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
Co-authored-by: Eksperimental <eksperimental@autistici.org>
Copy link
Member

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion I have is to simulate a slightly more complex example that however can be significantly more realistic.

My go to is once again something where the antipattern is configuring for example a base URL through app config

config :api_client,
  base_url: "..."

and the refactoring would be to pass it as an option:

APIClient.request(..., base_url: "...")

@lucasvegi
Copy link
Contributor Author

lucasvegi commented Sep 20, 2023

@whatyouhide What do you think of this example? If it's cool I'll adapt the text with a new commit

import Config

config :api_client,
  base_url: "https://api.github.com"

import_config "#{config_env()}.exs"
defmodule APIClient do
  def request(endpoint) when is_binary(endpoint) do
    HTTPoison.start
    base_url = Application.fetch_env!(:api_client, :base_url)  # <= retrieve parameterized value
    HTTPoison.get(base_url <> endpoint)                        # <= base_url: "https://api.github.com"
  end
end
#...Use examples with anti-pattern

iex> APIClient.request("/users/lucasvegi")  #<== OK!
{:ok,
 %HTTPoison.Response{
   status_code: 200,
   body: "...",
   headers: [ ... ],
   request_url: "https://api.github.com/users/lucasvegi",
   ...
 }}

iex> APIClient.request("/persons?_quantity=1")  #<== Failed attempt to access a different API!
{:ok,
 %HTTPoison.Response{
   status_code: 404,
   body: "{\"message\":\"Not Found\",\"documentation_url\":\"https://docs.github.com/rest\"}",
   headers: [ ... ],
   request_url: "https://api.github.com/persons?_quantity=1",  # <== base_url never changes!
   ...
 }}

Refactoring:

defmodule APIClient do
  def request(endpoint, options \\ []) when is_binary(endpoint) and is_list(options) do
    HTTPoison.start
    base_url = Keyword.get(options, :base_url, "https://api.github.com")  # <= default config of base_url == "https://api.github.com"
    HTTPoison.get(base_url <> endpoint)
  end
end
#...Use examples of the refactored code

iex> APIClient.request("/users/lucasvegi")  #<== OK with default config!
{:ok,
 %HTTPoison.Response{
   status_code: 200,
   body: "...",
   headers: [ ... ],
   request_url: "https://api.github.com/users/lucasvegi",
   ...
 }}

iex> APIClient.request("/persons?_quantity=1", [base_url: "https://fakerapi.it/api/v1"]) #<== OK with another API!
{:ok,
%HTTPoison.Response{
  status_code: 200,
  body: "{...\"data\":[{\"id\":1,\"firstname\":\"Taryn\",\"lastname\":\"Price\",...}",
  headers: [ ... ],
  request_url: "https://fakerapi.it/api/v1/persons?_quantity=1",
  ...
}}

@whatyouhide
Copy link
Member

@lucasvegi yes that's exactly what I had in mind. Few notes:

  • We don't need to mention HTTPoison specifically. Let's replace it with MyApp.HTTPClient.
  • We don't need to be verbose when showing return values as you do in your example, as it's not what we're talking about here.

Does that make sense?

@lucasvegi
Copy link
Contributor Author

def request(endpoint) when is_binary(endpoint) do
    HTTPoison.start
    base_url = Application.fetch_env!(:api_client, :base_url)  # <= retrieve parameterized value
    HTTPoison.get(base_url <> endpoint)                        # <= base_url: "https://api.github.com"
  end

So the definition of the request function would look something like this?

  def request(endpoint ...) do
    base_url = ...
    MyApp.HTTPClient.get(base_url <> endpoint)
  end

@whatyouhide
Copy link
Member

@lucasvegi yep!

@lucasvegi
Copy link
Contributor Author

@lucasvegi yep!

Great! I'll make some adjustments and commit

@josevalim josevalim merged commit 1ecdd2d into elixir-lang:main Sep 21, 2023
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants