To improve a function

Questions / Help
,
1

Hello,

I am still an Elixir newbie. To learn I keep on asking questions like this…
Can I improve this two functions? It feels like but I do not know how.

  defp get_url(team_id) when team_id > 0 do
    "a_url_removed_because_not_public" <>
      Kernel.inspect(team_id)
  end

  defp get_url(team_id) when team_id == 0 do
    "a_url_removed_because_not_public" 
  end

besides doing them as one-liners:

  defp get_url(team_id) when team_id > 0; do:  "a_url_removed_because_not_public" <> Kernel.inspect(team_id)

  defp get_url(team_id) when team_id == 0; do:  "a_url_removed_because_not_public"

It still looks as it contains too much redundant code.

1 Like
2

I’d add a guard is_integer(team_id) as any non numeric type is greater than 0.

Also, I’d use string interpolation rather than inspect.

Inspection in general is something I’d only use for logging, never to create a user facing string.

4 Likes
3

You might use

defp get_url(0) do ...
4 Likes
4

Quite possibly!
Here are a couple of questions for you, that might help you:

  • Are the URLs in the two function clauses the same? If they are, you could extract this out to either another function, or to a module attribute.
  • Instead of using Kernel.inspect (which is meant to only be used for logging/introspection), you probably want to use to_string, or string interpolation. Also, there is no reason to use Kernel. in front of a function, because the Kernel module is always (unless explicitly overridden) imported into a module.
  • Why have the get_ in the name of the function? Depending on the context, this might matter for the meaning of the function name but maybe it is fluff that does not add extra meaning.
4 Likes
5

Thanks, this is putting me forward :slight_smile:

6

Hm. I thought functions should be a verb as they are doing something with the data they get (I think that goes back to university days).

Background of my functions is that another function needs a url (like “Website Domain Names, Online Stores & Hosting | Domain.com”) and some additional paramenter (so “Website Domain Names, Online Stores & Hosting | Domain.com” if some condition is met (team_id is not 0).

7

This is not required. And if you really want to put a verb in there, get is about as vague as you can be, since it means so many different things :sweat_smile:. Using a verb like get or fetch as part of a function name would make more sense if you retrieve the URL value from somewhere else. In this case, where it is something the function itself calculates, I would just leave it out:

defmodule Team do
  def url(team_id) do
    ...
  end
end

Here, it is clear that Team.url(foo) will return an URL based on foo that has to do with a ‘Team’.

Of course, naming things quickly enters the realms of personal preference. It is very easy to spend all afternoon bikeshedding about it. It’s possibly the least important suggestion of all of the suggestions that people have given in this topic :wink: .

2 Likes
8

Allright. Thank you :slight_smile:

9

A follow up question:
I have to an request data from an foreign API for about 1 million times. So for every single team_id I have to contact an url, receive the data and insert (or update) the data as a new row in a database (if it meets certain conditions).

I know I can do this easily via for but wouldn’t that be a perfect fit for GenServer? That would give me a first practise on genserver…

10

I’d probably just make:

defp get_url(id) when is_integer(id) and id >=0 do
  "a_url_removed_because_not_public#{if(id > 0, do: id)}"
end

Or more readably as (since I like to pull out arg calculations onto their own lines:

defp get_url(id) when is_integer(id) and id >=0 do
  sid = if(id > 0, do: id, else: nil)
  "a_url_removed_because_not_public#{sid}"
end
2 Likes
11

And what about my question regarding using GenServer? Too ridiculous to comment on? :dizzy_face:

12

It would be fun to do this with GenServer as an exercice but…

  • It will probably become a bottleneck
  • There are better tools to do this in OTP : GenStage, Flow or Broadway