[erlang-questions] Orelse and andalso as short-hand for case

[zxq9@REDACTED]

For whatever reason, Richard's responses never made it into the archive.
I believe they are worthy of preservation.

-Craig

On 2018年7月25日水曜日 15時00分03秒 JST Richard O'Keefe wrote:
> Pierre Fenoli does not "understand the strong answers".
> I imagine that other people have had the experience of having to maintain
> someone else's code, and after a long struggle to disentangle unduly
> "clever"
> code found themselves shouting "Why did this offspring of an incestuous
> union between two male naked mole-rats not remove xer hands from xer
> genitals long enough to write down what xie actually meant and NOT WASTE
> MY RAPIDLY DWINDLING TIME?"
> 
> I am still nowhere near as good a programmer as I would like to be.
> I need to make my own code clearer.
> "What is hateful to you, do not do to others."
> If anyone catches me abusing short-circuit operations, my apologies
> in advance.
> 
> The andalso and orelse operators are defined in section 8.14
> http://erlang.org/doc/reference_manual/expressions.html#short-circuit-expressions
> of the Erlang reference manual.  It is clear from that that
> these operators originally required their right operand to yield
> true or false, and were only changed in order to allow code like
> 
> 
> ismember(X, [Y|Ys]) -> X == Y orelse ismember(X, Ys);
> ismember(_, []    ) -> false.
> 
> to be tail recursive.
> 
> It would be really nice if the Dialyzer would warn about misuses of
> these operators.  What about the 'legit example' we were offered?
> It is a blemish in an otherwise pleasant file in an impressive and
> useful project.  The only occurrence of 'andalso' in that 628 line
> file is a good example of when NOT to use 'andalso'.
> 
> task_by_pid(Pid, #state{tasks = Tasks}) ->
>     F = fun (_, #{worker_pid := WorkerPid}=Task, Acc) ->
>                 Pid =:= WorkerPid
>                     andalso throw({'task', Task}),
>                 Acc
>         end,
>     try maps:fold(F, 'undefined', Tasks)
>     catch 'throw':{'task', Task} -> Task
>     end.
> 
> which is 9 lines.  It garden-paths the reader into a
> sort of double negation.  The goal of F is *not* to return Acc,
> but to find a Task, and the throw is *not* there to report
> failure, but success.
> 
> This can be written, not only without 'andalso',
> but also without 'throw' and 'catch', as
> 
> task_by_id(Pid, #state{tasks = Tasks}) ->
>     task_with_matching_worker_pid(Pid, Tasks).
> 
> task_with_matching_worker_pid(Pid, [Task = #{worker_pid := Pid}|_]) ->
>     Task;
> task_with_matching_worker_pid(Pid, [_|Tasks]) ->
>     task_with_matching_worker_pid(Pid, Tasks);
> task_with_matching_worker_pid(_, []) ->
>     undefined.
> 
> which is also 9 lines.  To me this is a fairly obvious linear
> search for a matching list element, which leaves me free to
> think about things like whether a map from worker Pids to Tasks
> might not be a better data structure, and also lets me muse,
> "hmm, isn't there already a library function to look for the
> first matching list element?  Oh yeah."
> 
> task_by_pid(Pid, #state{tasks = Tasks}) ->
>     case lists:search(fun (#{worker_pid := Id}) -> Id =:= Pid end, Tasks)
>       of {value, Task} -> Task
>        ; false         -> undefined
>     end.
> 
> which is now 5 lines, and is how I'd have written it in the first place.
> 
> I honestly wasn't expecting this.  Given that this was Kazoo, I
> was expecting to have to mumble and shuffle and say "well, maybe
> OCCASIONALLY you can get away with it in otherwise really good
> code", but the abuse of 'andalso' DID turn out to be a 'code smell'
> pointing to contorted code that needed rewriting.
> 
> If, as would not be surprising, I have misunderstood thpe original
> code, that just proves my point that it was contorted.
>