Hello Community,
After reading a bunch of books and doing nothing , I finally decided to write something and came up with this https://github.com/chattes/extract_poi.
This is a CLI that downloads POI Data(Points of Interests) of a Country and Stores as JSON in a FIle.
Its mostly follows a fire and forget pattern to extract the data from Triposo.
This was needed for work anyway and hence decided to write it in Elixir.
I would love some feedback/pointers etc regarding any glaring anti-patterns or mistakes!
I am not a beginner programmer, but have started to explore Elixir as my tool of choice for writing any scripts/side projects.
Thanks and would love some feedback here or as issues in Github.
import IO.ANSI # As a general rule of thumb, avoid imports if you can. They pepper your code with _magic_ functions
alias PointOfInterest # You don't really need require, as PointOfInterest has no macros that need to be pre-compiled.
require Logger
You would have a config/config.exs with the following:
config :poi_cli,
base_url: "....someurl..."
There are also some issue with your project structure, but at this point I am still having my own dilema on how to organize one properly, so I will leave this advice for someone else to give
Do you alias IO, Enum and all the other stdlib modules the same way?
To be honest, I rarely read modules top2bottom, I read the functions that are in the module. And when I see an alias in a function call that I do not know already, I hover with the mouse and see its expanded alias or I could even Ctrl-click it and jump directly to the definition of the function called.
In general I try to avoid “noise” in the modules “prelude” as much as possible.
I would say Logger is a hair easier to keep your tests’ output tidy because it’s been special-cased as a “capture_log” tag on ExUnit.Case, but this is not a testing consideration directly because it doesn’t allow for assertions.
I disagree. You can specify how logger behaves per environment and you can even configure how different backends behave. Good luck doing that wit IO without re-inventing the wheel.
The tag-based special case is what doesn’t allow for assertions - it’s just to suppress log output when your test cases are passing.
That’s your prerogative, but you originally presented your position as un-nuanced objective fact when it is not so. At very least, now you’ve explained why you believe it to be so, which is a net improvement to the information available to the OP. That’s how you should’ve phrased it in the first place.
tag based special case? I didn’t use any tags afaik in my examples (@tag :some tag). Could you elaborate?
You can’t explain every little detail to everyone and still have a life and a job to do. My comments, as well the comments of anyone, represent their opinions and the solutions we defend for any given problems. If the OP or anyone else has questions, they are always welcome to ask for further clarification.
First of all you should have: my_app/lib/my_app.ex and my_app/lib/my_app/ + all your modules should be prefixed with: MyApp.. Your app name is :poi_cli, but you have only: PointOfInterest.MixProject, PointOfInterest and Poi.CLI which does not makes sense for me and it’s really problematic when using as dependency, because you are reserving for you 2 module namespaces (PointOfInterest.* and Poi.*) and 1 extra app name (poi_cli instead of poi or point_of_interest).
Secondly I would rewrite whole code using for, with and other ways to make everything code much smaller, for example:
def get_pois_for_country(%{country: country, filename: filename}) do
country
|> get_cities
|> pmap(&get_poi(&1))
|> List.flatten()
# |> Enum.filter(&match?({:ok, _}, &1))
|> Enum.filter(fn
{:ok, _value} -> true
_ -> false
end)
|> Enum.map(&elem(&1, 1))
|> write_poi(filename)
|> (fn
{:ok, data} -> IO.puts("Wrote #{Enum.count(data)} records succesfully")
_ -> IO.puts("Failed to write records to File")
end).()
end
# better:
def get_pois_for_country(%{country: country, filename: filename}) do
poi = country |> get_cities |> pmap(&get_poi/1) |> List.flatten()
result = for {:ok, data} <- poi, do: data
result |> write_poi() |> get_message() |> IO.puts()
end
defp get_message({:ok, data}), do: "Wrote #{Enum.count(data)} records succesfully"
defp get_message(_), do: "Failed to write records to File"
Then you would need to fix some problems. For example write_poi/1 function always returns {:ok, result} and it could fail (by File.write!) - you probably wanted here to catch fails (using File.write). For now it’s your error check in get_pois_for_country/1 function have no sense, because this clause would never hit.
Finally I recommend to use Elixir's builtin formatter.
This was referring to the third and fourth links of my first reply. I wasn’t trying to say that you, @Fl4m3Ph03n1x, had suggested this, just trying to describe one of the adjacent features of the ExUnit.CaptureLog module I had just mentioned in the same post.
Specifically, you can write a test case that looks like so, and no* output from Logger macros will appear during test runs unless a case fails. The full log history will be shown if a test does fail.
@tag capture_log: true
test "function that emits logs" do
assert :ok = Module.function_that_logs()
end
You can additionally apply this to an entire test module by using @moduletag capture_log: true, or by default using the syntax in the ExUnit.Case (fourth) link.
Caveat:
Logger calls from sibling Elixir processes are not always altered, see the capture_log/2 docs for details.
(Mods are welcome to splinter this tangent of discussion off from this thread, if they wish)
Thanks for the feedback. This is really why i shared as all code in general somewhat works, but we need to get the best practices to avoid potential pitfalls.
Thanks for the feedbacks till now. This is a great way for beginners to learn quickly i think…
Showcase -> Feedback -> Refactor -> Repeat.
Thanks Guys!
Keep them coming
, I finally decided to write something and came up with this https://github.com/chattes/extract_poi.
