exercism / elixir-analyzer

GNU Affero General Public License v3.0
30 stars 32 forks source link

Analysis of Basketball Website #305

Closed altuntasfatih closed 2 years ago

altuntasfatih commented 2 years ago

I received the following feedback on my answer to the Basketball Website problem:

"In this exercise, there is no need to handle nil values explicitly. The Access Behaviour is implemented for nil."

If I do not handle the Nil case, it continues to iterate until the finish path array. However, in my opinion, that is a redundant iteration because the value is already nil and never changes. Also, many community solutions are like that; they are iteration redundantly

What is your opinion about that?

Here is my solution also ` defmodule BasketballWebsite do def extract_from_path(data, path) when is_binary(path) do [k | tail] = keys(path) extract(data[k], tail) end

defp extract(nil, _), do: nil defp extract(data, []), do: data defp extract(data, [head | tail]), do: extract(data[head], tail)

def get_in_path(data, path) do Kernel.get_in(data, keys(path)) end

defp keys(path) do String.split(path, ".", trim: true) end end `

arconautishche commented 2 years ago

You could probably simplify your extract function and remove the nil issue like this:

defp extract(data, [only_key]), do: data[only_key]
defp extract(data, [head | tail]), do: extract(data[head], tail)

Although I'm not sure your original solution isn't just as valid.

altuntasfatih commented 2 years ago

@arconautishche, My solution is correct, but I am trying to point out another thing.

The feedback says you should not handle the nil case because the Access module already handles it like in your simplified version. In your simplified version( also many other community solutions), the code iterates all path items even when data is nill.

In brief, the above feedback directs people to a solution that is not very proper.

angelikatyborska commented 2 years ago

@altuntasfatih You're absolutely right. I didn't notice that the pattern matching on nil has a purpose, at a first glance it looked unnecessary. We should remove that piece of feedback from the analyzer. Would you want to open a PR for that?

altuntasfatih commented 2 years ago

@angelikatyborska I would like to open pr for that