[erlang-questions] Coming Back (maybe improving lists:reverse/1)

[Richard A. O'Keefe]

On 8/10/2015, at 1:20 pm, Ivan Carmenates Garcia <co7eb@REDACTED> wrote:

> For example this is one of the algorithms, I optimize it as well as I could:

Start by optimising the documentation.
>  
> Considering here that the order of the fields is very important!.
>  
> %% -------------------------------------------------------------------
> %% @private
> %% @doc
> %% Parses the specified list of full fields into a string containing
> %% all full fields separated my comma.
> %% example:
> %% <pre>
> %%   parse_full_fields([{users, '*'}, name, {roles, [id, level]},
> %%       {users, name, alias}], fun get_postgres_operator/2) ->
> %%       {"users.'*',name,roles.id,roles.level,users.name AS alias",
> %%           [users, roles]}.
> %% </pre>
> %% Returns `{[], []}' if called with `[]'.
> %% @throws {error, invalid_return_fields_spec, InvalidForm :: any()}
> %% @end
> %% -------------------------------------------------------------------

Parsing turns a string into structure.
Turning structure into a string is the OPPOSITE of parsing.
You are not parsing but UNparsing.

> -spec parse_return_full_fields(FullFieldsSpecs, Separator, OperatorFinder) -> Str when
>     FullFieldsSpecs :: proplists:proplist(),
>     Separator :: string(),
>     OperatorFinder :: fun(),
>     Str :: string().

This is seriously confusing.
The thing about a proplist, as we've recently discussed in this
list, is that it contains
 - atoms, where 'x' is equivalent to {'x',true}
 - pairs {Key, Value}
 - junk, which is completely ignored.
While I don't fully understand your example, it's clear
that    name    is NOT equivalent to {name,true} and
that    {users, name, alias}   is NOT ignored as junk.
So whatever you have, it certainly isn't a proplist.

Finally, your -spec describes a function with three arguments,
but the example in your comment has only two arguments.

It looks like you are mapping
    'atom' -> "atom"
    {'table', 'field'} -> "table.field"
    {'table', ['f1',...,'fn']} -> "table.f1" ... "table.fn"
    {'table', 'field', 'alias'} -> "table.field AS alias"

It would seem more logical to have
    field() = atom() | {atom(),atom()}.  % {name,alias}
    fields() = field() | list(field()).
    slot() = atom() | {atom(),fields()}. % {table,fields}
so that it's obvious how to put an alias inside a list of fields.

-module(goo).
-export([
    tables/1,
    unparse_return_full_fields/1,
    unparse_slots/1
 ]).

unparse_return_full_fields(Slots) ->
    {lists:flatten(unparse_slots(Slots)), tables(Slots)}.

tables([{Table,_}|Slots]) ->
    Tables = tables(Slots),
    case lists:member(Table, Tables)
      of true  -> Tables
       ; false -> [Table|Tables]
    end;
tables([Name|Slots])
  when is_atom(Name) ->
    tables(Slots);
tables([]) ->
    [].

unparse_slots([Slot|Slots]) ->
    [ unparse_slot(Slot)
    | unparse_remaining_slots(Slots) ].

unparse_remaining_slots([]) ->   
    "";
unparse_remaining_slots(Slots) ->
    ["," | unparse_slots(Slots)].

unparse_slot({Table,Fields}) ->
    unparse_fields(Fields, atom_to_list(Table));
unparse_slot(Name)
  when is_atom(Name) ->
    atom_to_list(Name).

unparse_fields([Field|Fields], Table) ->
    [ unparse_fields(Field, Table)
    | unparse_remaining_fields(Fields, Table) ];
unparse_fields({Name,Alias}, Table) ->
    [Table, ".", atom_to_list(Name), " AS ", atom_to_list(Alias)];
unparse_fields(Name, Table)
  when is_atom(Name) ->
    [Table, ".", atom_to_list(Name)].

unparse_remaining_fields([], _) ->
    "";
unparse_remaining_fields(Fields, Table) ->
    ["," | unparse_fields(Fields, Table)].

With that definition, we get

1> c(goo).
2> goo:unparse_slots([{users, '*'}, name, {roles, [id, level]},
   {users, {name, alias}}]).   %% Note difference here.[["users",".","*"],
 ",","name",",",
 [["roles",".","id"],",",["roles",".","level"]],
 ",",
 ["users",".","name"," AS ","alias"]]

This is a tree of strings, not a string.  But for many
purposes, it's just as good.  (It's called an iolist.)
Flattening it gives
"users.*,name,roles.id,roles.level,users.name AS alias"
3> goo:unparse_return_full_fields([{users, '*'}, name, {roles, [id, level]}, {users, {name, alias}}]).
{"users.*,name,roles.id,roles.level,users.name AS alias",
 [roles,users]}

You will note that the functions above don't have any
use for reversing a list.  And I must repeat that for
many purposes, such as sending text to another OS process,
there isn't any *need* to flatten the tree of strings.

Even in your code,

> parse_return_full_fields(FullFieldsSpecs, Separator, OperatorFinder) ->
>     {ParsedFields, TableNames} =
>         parse_full_fields2(FullFieldsSpecs, [], [], OperatorFinder),
>     {string:join(lists:reverse(ParsedFields), Separator), TableNames}.

there isn't actually any need to do this:

    my_reverse_join([X|Xs], Sep) ->
        my_reverse_join(Xs, X, Sep);
    my_reverse_join([], _) ->
        "".

    my_reverse_join([], Acc, _) ->
        Acc;
    my_reverse_join([Y|Ys], Acc, Sep) ->
        my_reverse_join(Ys, Y++Sep++Acc, Sep).

9> goo:my_reverse_join(["harry","deacon","thom","avery"]).
"avery,thom,deacon,harry"


We see two ideas here which are likely to be applicable
to your code in context.

(1) It may be possible to *eliminate* a call to lists:reverse/1
    by fusing it with the function the result is being passed to.

(2) It may be possible to *eliminate* appends (++) by constructing
    an iolist (a tree of strings) instead of a string.  For example,
    io:put_chars/2 wants a unicode:chardata().
    http://www.erlang.org/doc/man/unicode.html#type-chardata
    http://www.erlang.org/doc/man/io.html#put_chars-2