[erlang-questions] Request feedback on example project

[zxq9]

Hi Kostis!

Thank you for catching those!

On 2016年2月4日 木曜日 01:20:06 Kostis Sagonas wrote:
> Some comments concerning the specs that you have written.
> 
> In zuuid_man.erl, I see:
> 
> -spec init(term()) -> {ok, term()}.
> init(_) ->
>      {ok, #s{}}.
> 
> and my immediate reaction is: well, this function does not return a 
> ok-pair with any term in the second position; instead, it returns an 
> #s{} record in that element.  The question that comes to my mind next, 
> is why did the author of the code write that?  Does (s)he simply want to 
> hide the return structure?  This is not the proper way to do this.
> 
> Further down I see:
> 
> -spec v1(State) -> {UUID, NewState}
>      when State    :: #s{},
>           UUID     :: zuuid:uuid(),
>           NewState :: #s{}.
> 
> why not introduce a state() type for #s{} and use it throughout this 
> module?  Also, if you want other modules to not be able to inspect/rely 
> on the structure of the state record, declare it as opaque.

Indeed!

I've made this change. As for the spec for init/1, I had left a template
spec line in there and never went back to tighten it -- oops. I suppose that
is actually *worse* than giving Dialyzer no type at all.

As for opaqueness...  It wasn't about exporting the type or not (I can't
recall ever exporting a gen_server's state type, come to think of it...?)
but simply about having been lazy and not reviewing my typespecs again. I
had simply run Dialyzer over the project, everything checked out, and...

Here's to the power of the critical eye over blindly trusting automation.
:-)

> Small type lies also exist in zuuid.erl.  The function:
> 
> -spec read_uuid_string(list()) -> uuid() | bad_uuid.
> read_uuid_string(UUID) ->
>      Parts = string:tokens(UUID, "{-}"),
>      case lists:map(fun length/1, Parts) of
>           ...
> 
> clearly is not meant to accept any list; it's expecting a string().
> While at it, I suggest to use a list comprehension instead of lists:map.
> 
> Similar changes need to happen in read_map_string/1 further below.

Indeed. Fixed. Thank you.

As for using list comprehensions VS lists:map/2 and lists:filtermap/2...
I have noticed that I'm nearly as likely to write lists:map/2 as I am to
write a list comprehension -- except in the case of unpacking tuples, where
I nearly always write list comprehensions.

I don't have a real reason for this and like both ways just fine. I am
wondering, though, if there is any general consensus on this?

comprehensions > map/filtermap ?

I switched both cases to list comprehensions just because, but don't really
know if I care one way or the other.

> Finally, it's debatable whether it's a good idea for functions to return 
> error values (e.g. 'bad_mac') instead of just crashing on inputs they 
> are not supposed to handle, but I guess this is a matter of design.

We have a convention (a guideline, not a hard rule, though) for this:

* Any pure function that touches "internal" (as in, already scrubbed/checked
values) data only always crashes on bad data, and is written to return a
naked value instead of a tuple (to enable clean function composition). (I
strive to make as much of a codebase fall into this category as possible.)

* Any pure function that accepts external data and is named 'read_*' will
always return an {ok, Value} tuple on success where Value's type is now
guaranteed, and an atom like 'bad_*' or {error, Reason} on bad data, but
not crash. (This is the case read_uuid/1 and read_mac/1 and their helper
functions fall in to.)

* Any pure function named Module:to_[type] or Module:from_[type] will return
a naked value or crash on bad data. Forests of these tend to surround ADT
modules.

* Directly side-effecty functions always return an {ok, Value} tuple or
'ok' on success, {error, Reason} if the external resource is manageable
but merely unavailable (usually temporary), and crash in any other
situation. Any side-effecty function is expected to crash at any time,
though, just on principle.

I don't think I've ever even written that out in English before... I should
certainly put that somewhere as a reference.

On that note, I am very curious to know if anyone has a different or better
idea for function return/outcome guidelines -- usually this aspect of
function outcomes seems to just be left up in the air.

> Hope these help,

Helped quite a bit. Thanks again for your time and attention.

-Craig