The code is from Phoenix Guardian example. I think it’s cleaner than using multiple case logics but the compiler give you this annoying warning. Is the compiler right and is there a better way of writing it?
defp create_user_from_auth(auth, current_user, repo) do
user = current_user
if !user, do: user = repo.get_by(User, email: auth.info.email)
if !user, do: user = create_user(auth, repo)
authorization_from_auth(user, auth, repo)
{:ok, user}
end
warning: the variable "user" is unsafe as it has been set inside a case/cond/receive/if/&&/||. Please explicitly return the variable value instead. For example:
case int do
1 -> atom = :one
2 -> atom = :two
end
should be written as
atom =
case int do
1 -> :one
2 -> :two
end
You can (hopefully, because I did not test it) do something like this
defp create_user_from_auth(auth, current_user, repo) do
user =
if !current_user, do: repo.get_by(User, email: auth.info.email), else: nil
user =
if !user, do: create_user(auth, repo), else: user
case user do
nil ->
{:error, "reason for an error, ie: `user does not exist`"}
_ ->
authorization_from_auth(user, auth, repo)
{:ok, user}
end
end
I might be totally wrong here though
Anyway the compiler is warning you, because, setting rest of code aside look at this line: if !x, do: y = z
It’s similar to second line in your function. y is set only if x == false, it won’t be set if x == true and if you will try to use y later in code, when it is not yet set… you got a problem
So similarly if !user, do: user = repo.get_by(User, email: auth.info.email) user is set only when !user which means user == false and what about when user == true ? Then user won’t be set at all, and this may give you a problem Compiler is not smart enough to understand this code like a human does. So although from my point of view your code seems to be legit an will work, compiler just warns you, to make you aware of potential problems.
@grych That fixes it in one go
I still don’t think the compiler should be outputting a warning though. There is always a value in user from the first statement.
@omin: check the “Deprecation of imperative assignment” topic in http://elixir-lang.org/blog/2016/06/21/elixir-v1-3-0-released. This warning appears only in Elixir 1.3. And in my opinion, it is great! We should avoid such constructions in functional languages, even if they allow it.
Everything is an expression; everything will return some value.
As others has said, the compiler will warn you if you construct something like one would write in an imperative language. All of a sudden branching out and defining a variable for later use, outside the block, is hard to read and confusing.
Compiler is not smart enough to understand this code like a human does. So although from my point of view your code seems to be legit an will work, compiler just warns you, to make you aware of potential problems.
Thanks guys
