[erlang-questions] Error handling with good looking code?

[Garrett Smith]

Hi Attila,

On Thu, Nov 3, 2011 at 8:08 AM, Attila Rajmund Nohl
<attila.r.nohl@REDACTED> wrote:
> Hello!
>
> I have this code (cut out some non-interesting pieces):
>
> upload_trace_record_file2(Host, Usr, Pwd, RemDir, NetTraceRef) ->
>    case sftp_client:open(Host, ?SFTP_PORT, Usr, Pwd) of
>        {ok, Pid} ->
> ....
>            case sftp_client:send(Pid, TRFile, RemTRFile) of
>                ok ->
>                    ok;
>                {error, Reason} ->
>                    {error, sftp_client:format_error(Reason)}
>            end,
>            sftp_client:close(Pid),
>            file:delete(TRFile);
>        {error, Reason} ->
>            {error, sftp_client:format_error(Reason)}
>    end.

Aren't you just talking about this:

 case open_client(Info) of
     {ok, C} ->
         Result = send_files(C, Files),
         cleanup(C, Files),
         Result
     {error, Err} -> {error, Err}
 end

Alternatively,

 case open_client(Info) of
     {ok, C} ->
         try
             send_files(C, Files)
         after
             cleanup(C, Files)
         end
     {error, Err} -> {error, Err}
 end

> update_trace_record_file(TRFile, NetTraceRef) ->
>    case exml:read_file(TRFile, [{space, normalize}]) of
>        {ok, ReadExml} ->
>            {traceCollecFile, Attributes, ChildList} = ReadExml,
> ...
>            file:write_file(TmpTRFile, TraceText),
>            TmpTRFile;
>        _ ->
>            throw({error, bad_xml_file})
>    end.

The general pattern seems fine:

 case read_file(Info) of
     {ok, File} ->
         write_file(File),
         File;
     {error, Err} -> throw(Err)
 end

The alternative is to return a tagged value:

 case read_file(Info) of
     {ok, File} ->
         write_file(File),
         {ok, File};
     {error, Err} -> {error, Err}
 end

Some lesser observations:

- You're losing valuable information by ignoring the error from
exml:read_file/2 - that should probably be in the thrown term

- Use exit(Reason) rather than throw if its part of the API (i.e.
anyone outside your code might handle it) - exit will propagate up the
stack and cause the process to terminate with the term provided minus
the call stack

> There are at least two obvious bugs in the error handling (the error
> from sftp_client:send is not used; the update_trace_record_file throws
> an exception instead of returning an error), but it made think about
> how to handle the errors properly in this code. The "let it crash"
> philosophy doesn't really work here, because we need to produce a
> meaningful error message to the user and the stack trace is not a
> meaningful message to them. I was thinking something like this:
>
> upload_trace_record_file2(Host, Usr, Pwd, RemDir, NetTraceRef) ->
>    try
>        {sftp_client, {ok, Pid}} = {sftp_client,
> sftp_client:open(Host, ?SFTP_PORT, Usr, Pwd)},
> ....
>        {sftp_client, ok} = {sftp_client, sftp_client:send(Pid,
> TRFile, RemTRFile)},
>        sftp_client:close(Pid),
>        file:delete(TRFile)
>   catch
>        error:{badmatch, {sftp_client, Reason}} ->
>            {error, sftp_client:format_error(Reason)};
>        error:{badmatch, {exml, _}} ->
>                  {error, bad_xml_file}
>    end.
>
> update_trace_record_file(TRFile, NetTraceRef) ->
>    {exml, {ok, ReadExml}} = {exml, exml:read_file(TRFile, [{space,
> normalize}])},
>    {traceCollecFile, Attributes, ChildList} = ReadExml,
> ...
>    file:write_file(TmpTRFile, TraceText),
>    TmpTRFile.
>
> but it looks somewhat ugly. Three "assignments" and function calls are
> hidden away in tuples, so it's a little hard to read, even though the
> rest of the code is simpler. Do you have any other ideas?

If I were to restate, you're suggesting that this:

 case foo() of
     {ok, Val} -> good(Val);
     {error, Err} -> bad(Err)
 end

be written like this:

 try
     {foo, {ok, Val}} = {foo, foo()},
     good(Val)
 catch
     error:{badmatch, {foo, {error, Err}}} ->
         bad(Val)
 end

When used for good, Erlang lets you write clear, logical, maintainable
code. If your eyes start to bleed when reading code (as is the case
here for me), push through the confusion / fuzziness and work until
the code is obviously descriptive of the problem you intend to solve.

Just my 2 cents :)

Garrett