Looking for code review for my first Library!

Hi, it’s a nice code for the first project. Here are the things I’ve noted during reading

Not important things

• mix format. Use spaces or tabs, not both

• Translate this

            response
            |> get_template_id_from_response
  

  to get_template_id_from_response(response)

• Do not refer to the functions in module like Carbonex.function, use just function.

• Translate

    case decode_json(response) do
      {:ok, map} -> map["data"]["templateId"]
      _ -> nil
    end
  

  to

    case decode_json(response) do
      {:ok, %{"data" => %{"templateId" => id}}} -> id
      _ -> nil
    end
  

  By the way, the code above occurs twice, so I’d suggest to DRY it.

• You can pipe into case, so instead

  result = Jason.encode(data)
  case result do
  

  you can write

  data
  |> Jason.encode()
  |> case do
  

Important things and design

• More tests. To test the HTTP interactions, you can use awesome Bypass library.

• Do not use System.get_env. I’d suggest just to create some structure which holds url, version and token for the carbone.io. This will make the code more pure (in functional sense) and easier to test

• Provide function which starts finch with given url, version and token.

• Learn specs. Proper and precise type annotations are better than any documentation.

• I think that get_document function should return the document, but not the HTTP response structure.

• create_multipart function checks the existence of a file, while it has already been checked in add_template function

Hi hissssst,

thanks a lot for reviewing my code!
I’m working on the implementation of the differents points you have described.
I will come back here when I think I’m done with!

Nice review! One thing to note is that piping into a case has been described as a bad pattern. The suggested solution is to extract the cases into a function (with multiple heads).

Benefits:

1. Function name can be explanatory about what’s happening
2. Easier to test the ‘cases’

Yeah, I’ve seen @keathley 's blog post before. But to be honest, I think that it is so minor, that it’s not even worth talking about. I’ve included it into list of not important things more like a “you can do this” than “you should do this”. And the benefits you’ve described are so general that they’re applicate to every pipe.

And considering the post, it’s so much opinionated, so I don’t think that it’s fair to reason about it in a terms of a good or bad practice. It’s more like a set of personal preferences.

I mean, I don’t like tap and then even if they increase readability. And I don’t like Access, because it has problems with structures. But I’ve seen a lot of great projects where people have used Elixir in a very weird way which I do not like.

Check out this article on mocking via explicit contracts by the man himself: Mocks and explicit contracts - Dashbit Blog

The use case in the article is about testing external APIs.

Piping into case is absolutely a matter of taste and I say do it if you want, but I don’t agree that the presented logic applies to any pipeline. I find it significantly clearer when the data structure that goes into a pipeline is the same data structure that comes out the other end. There are, of course, always exceptions, but that’s my rule of thumb. The example from the article is subtle, but in the pipeline version we never explicitly see that the data was ever transformed into a changeset, so it’s nice to explicitly call that out.

Since Chris is already in this thread, maybe he can confirm if I’m on the right track or not, but I’m not gonna tag him as I’m sure he’s sick of talking about it In any event, that’s my reasoning for not case-piping but again, it’s all a matter of taste!

Hi, sorry for adding to such a long thread

[1]

I do not like APIs that require string keys / other than camel case as input, such as the request-body using convertTo, for example

request_body = %{"convertTo" => "pdf", "data" => data}
Carbonex.create(finch_name, "path/to/template.odt", request_body)

should ideally be

Carbonex.create(template_path, data, convert_to: :pdf)

In general if you read Elixir source code a lot you will find that folks usually use camel case only, we do not do title case much, so it would be good to make it consistent.

Also exposing Finch as your HTTP API is probably not ideal (where does finch_name come from, etc) better to make it configurable if you have to but otherwise have a default API that does not require plugging this information.

Case in hand ExAws uses hackney but doesn’t ask you what pool, it just uses the :ex_aws pool.

[2]

README needs work, show me how to actually use the library and I don’t want to see stuff that is standard such as “Documentation can be generated with ExDoc and published on HexDocs” which is just part of the original boilerplate.

The README should describe what is available in the library and how to use it. If I copy and paste each chunk of code into iex it should execute, which it does not right now. Also it would seem that carbone.io would require an API key, but the way to configure it is unclear. This must be addressed.

I am not to impose a rigid structure, but it is my opinion that for any library, the README should include the following:

1. Name of the library
2. Succinct description of the problem it solves (do not write a nerd essay, keep it under 100 words)
3. If the library solves a common problem, explanation of the problem and analysis of all equivalent solutions, justification for using this library over others (this can be skipped if writing first-party platform SDKs, otherwise especially if the library is algorithmic or data-intensive, it would be good to add some benchmark results)
4. Installation & configuration (the “add this to mix.exs” section comes in here, also any environment variables & key configuration should be included, once I’ve read this section I should have everything needed to get the library integrated)
5. Some examples / usage (can be part of hexdocs instead)

And if you are nice then add changelogs, acknowledgements, reference, etc.

Link to hexdocs should be possible to find within the first 1 second of looking at the README.

[3]

For symmetry reasons, you might want to instead call the function Carbonex.render vs Carbonex.create the latter has no direct counterpart in the API of the service. In general when writing an Elixir SDK for a service you should match the name of whatever method/function published by the service.

So I would expect something like this

@type option :: {:convert_to, :pdf}
@type data :: binary() | map()
@spec render(template_name :: string(), data :: data(), options :: list(option())) ::
  {:ok, document_data :: binary()} | {:error, reason :: term()}

Note how we are typing the option as a 2-arity tuple, this is so the options can be passed in as a Keyword and its type will be checked. If you write it as a map it will be messier.

You can check out Stripe and how they intersperse different SDKs together alongside with cURL, the bottom line here is that integration engineers are very busy and you need to make sure your Elixir SDK is aligned with the service as much as possible.

[4]

carbonex/templates/Sample_template.odt

Wrong capitalisation, should be

carbonex/templates/sample_template.odt

Sweat the small stuff

[5]

See Carbonex requires CARBONE_TOKEN to be set in the environment, I would think that this is an Elixir anti-pattern and folks would be happier if you wrapped the configuration in a context object etc.

Read Library guidelines — Elixir v1.21.0-dev for a bit and again compare how ex_aws is configured… Will users want to use more than 1 token with your library? If no then you can keep what you have, or change it so you get it from Config.

Otherwise, you have to do something like this

defmodule Carbonex.Environment do
  @enforce_keys ~w(token)a
  defstruct token: nil
end

defmodule Carbonex do
  def render(environment, data, options) do
    …
  end
end

Which will be used thusly

environment = Carbonex.Environment{token: "…"}
Carbonex.render(environment, …)

Again use own judgement

[6]

Lastly from a maintainability point of view, does your platform publish OpenAPI v3 Specs? If so, consider code generation vs hand-rolling code

Thank you to all of you to took time to read my code and to give me advices for improvements! I have a lot to do and to learn !

@evadne :

convertTo come from the API of Carbone.io. What do you mean by “my platform” ?
As you can see on their API-Documentation page they expect a convertTo in the request body.

At work for a project I had to consume their service. I have written a lib in PHP for one of our projects and for me it was I guess the best project to start with to learn a new language.

I am aware of code generators, we use them for some Web Services at work, but I think this is not the right way for me if I want to learn a new language.

I don’t have really experience(s) with functional languages other than one online course about Elixir for 2 years I guess

I think the idea (which I agree with) is that an Elixir client library should do the job under the hood of translating the service API’s expectations to a more idiomatic Elixir interface.

This

request_body = %{"convertTo" => "pdf", "data" => data}
Carbonex.create(finch_name, "path/to/template.odt", request_body)

is sort of a thin wrapper around finch, where this Carbonex.create(template_path, data, convert_to: :pdf) abstracts away the underlying service and HTTP implementation details.

@msimonborg ah… I understand! thank you for clarify this point!