Analysis of New Passport

[Vagab]

Hey guys. So, given this additional code:

def get_new_passport(now, birthday, form) do
  with {:ok, timestamp} <- enter_building(now),
       {:ok, manual} <- validate_no_coffee_break(now),
       counter = manual.(birthday),
       {:ok, checksum} <- stamp_form(timestamp, counter, form),
       number <- get_new_passport_number(timestamp, counter, checksum) do
    {:ok, number}
  end
end

defp validate_no_coffee_break(now) do
  case find_counter_information(now) do
    {:coffee_break, _} -> {:retry, now |> NaiveDateTime.add(15 * 60)}
    ok -> ok
  end
end

Analyzer hits me up with:

This exercise is about learning how to "follow the happy path", with the special form with, which lets you deal with errors at the end. Use with in your implementation of get_new_passport/3.

I understand it wants me to use else, but in this case I think it's actually more readable and idiomatic(docs)

[angelikatyborska]

Hi! Thanks for reporting. So technically this comment appears because we have a check that case shouldn't be used, there's no check that with must come with an else. If you wrote validate_no_coffee_break with a cond or if instead, the comment wouldn't appear 🙈 Confusing...

Our intention was indeed to make everyone use an else clause for the purpose of learning that it exists. The "bad use of else" that you linked mentions:

Note how we are having to reconstruct the result types of Path.extname/1 and File.exists?/1 to build error messages.

That doesn't apply to this exercise's desired solution. There are no type assertions on anything returned by find_counter_information, just simple pattern matching. Matching on {:coffee_break, _} is no different than matching on {:ok, _}. Thus I am not convinced that your solution is any more idiomatic - it's equally valid, but it doesn't fulfill the goal of learning about using else with with. Of course if you already feel comfortable with Elixir and know you prefer your way, you're more than welcome to ignore the analyzer feedback. We write it mostly to ensure that newcomers actually use the learning exercises to learn what they need to learn 😉.

That being said, the current situation that using a case anywhere in your solution triggers this comment is not ideal. It would be better to have a check that else is used, or at the very least the comment triggered by using case should be a different comment that the comment triggered by not using with because at the moment it's not possible to figure out that that's the cause.

[Vagab]

Our intention was indeed to make everyone use an else clause for the purpose of learning that it exists Makes sense

There are no type assertions on anything returned by find_counter_information, just simple pattern matching. Matching on {:coffeebreak, } is no different than matching on {:ok, _}. Thus I am not convinced that your solution is any more idiomatic - it's equally valid, but it doesn't fulfill the goal of learning about using else with with.

I see your point. Ok

That being said, the current situation that using a case anywhere in your solution triggers this comment is not ideal. It would be better to have a check that else is used, or at the very least the comment triggered by using case should be a different comment that the comment triggered by not using with because at the moment it's not possible to figure out that that's the cause.

Agreed. I would suggest though not checking that else is used though, since it's perfectly fine to use just with in this case like I did, and it still enforces the student to think about else anyway which fullfills the purpose of this exercise.