[erlang-questions] wierd outome on my match method

[Richard A. O'Keefe]

On 1/02/2015, at 3:31 am, Roelof Wobben <r.wobben@REDACTED> wrote:

> Thanks,
> 
> Everything is working now.
> Finnaly I have solved the whole exercise like this :

Where are the <strong>comments</strong>?

For every file you write, you want a header something like this:

%%% What: db.erl
%%% Who : Roelof Wobben'
%%% When: 2015-02-01
%%% Why : Solving $exercise on page $page in $book.

When you learn about EDoc markup, you might switch over to that
style, or you might prefer to use -attributes.

> 
> -module(db).
> 
> -export([new/0, destroy/1, write/3, read/2, delete_key/3, delete/2, match/2, match_key/3]).

A personal suggestion here: lay out the functors one per line in some useful
order.  My preference is for alphabetic order.  So

-export([
    delete/2,
    delete_key/3,
    destroy/1,
    match/2,
    match_key/3,
    new/0,
    read/2,
    write/3
 ]).

That way when you are looking to see if something is exported, you can just
scan your eye down, keeping focus on the same column, and you can instantly
tell when you have gone past what you are looking for.

> new() ->
>  [].
> 

This does not make a new anything.  It returns an empty list, but just
like the number 0, the empty list always exists.

> destroy(_Db) ->
>  ok.

This does not destroy anything.  It does nothing but waste time.
Even if an exercise tells you to do something pointless, YOU should have
a comment that shows that YOU know it is pointless.
> 
> write(Key, Element, Db) ->
>   [ {Key, Element} | Db ].

I have pointed out previously that this is ONLY tolerable when you know
that you are adding a key not already in the list.  YOU need to make this
clear in a comment.

> read(_Key, []) ->
>  {error, "Instance"};
> 
> read(Key, [Head | List]) ->
>  case element(1, Head) of
>    Key -> {ok, element(2, Head)};
>    _ -> read(Key, List)
>  end.

This is clumsy.  You have switched from saying "Db" to saying "List", for
no apparent reason.  It will be perfectly happy with triples; if that's
what you _want_, then this makes sense, but if you _want_ to accept
only pairs, that's what you should match for.  Better as

read(Key, [{K,V}|Db]) ->
    case K
      of Key -> {ok,V}
       ; _   -> read(Key, Db)
    end.

By the way, since commas and semicolons can be hard to tell apart,
and since commas go at the end of lines, I *strongly* suggest NOT
putting semicolons at the end of lines except for function clauses.

> 
> delete_key( _Key, [] , List2) ->
>  List2 ;
> 
> delete_key( Key, [Head | Tail], List3 ) ->
>  case element(1, Head) of
>    Key -> delete_key(Key, Tail, List3 );
>    _ -> delete_key(Key, Tail, [Head | List3] )
>   end.

delete_key(Key, Db1, Db2) =
    lists:reverse(
        [X || X <- Db1, if element(1, Head) =:= Key -> true ; _ -> false end])
    ++ Db2.

Now this is particularly nasty, so I wonder if you really meant that reversal
to happen.  Suppose we
start with    []
add x -> 1    [{x,1}]
add x -> 2    [{x,2},{x,1}]
add y -> 3    [{y,3},{x,2},{x,1}]
add y -> 4    [{y,4},{y,3},{x,2},{x,1}]
call that     Db0.

At this point, read(x, Db0) -> 2, read(y, Db0) -> 4.

delete_key(x, Db0, [])
call that     Db1 = [{y,3},{y,4}].

Now read(y, Db0) -> 3, instead of 4.  Deleting the value(s) for
x has *changed the result returned for an unrelated key*.

Do you really want that?

I strongly suspect that delete_key/3 should not be exported at all,
that it was meant to be a building block for delete/2.

> 
> delete( _Key, [] ) ->
>  {error, "Instance"};
> 
> delete(Key, List) ->
>  match_key (Key, List, []).

Surely this was meant to call delete_key(Key, List, []).
Also, there appears to be a problem.
Working with Db0 from the previous example,

delete(z, []) -> error
delete(z, Db0) -> Db0.

That is, deleting a key that is not present will result in
an error message if and only if there are NO keys.

It is hard to believe that this was intentional.

Here's something that
- does not change the order of the pairs in the list;
- keeps track of whether it has found a matching key or not;
- reports an error if and only if the key was not found,
  no matter how many keys *were* found.

delete(Key, Db) ->
    delete(Key, Db, not_found).

delete(Key, [{Key,_}|Db], _) ->
    delete(Key, Db, found);
delete(Key, [Pair = {_,_}|Db], Found) ->
    [Pair | delete(Key, Db, Found)];
delete(_, [], found) ->
    [];
delete(_, [], not_found) ->
    {error,"Instance"}.


> 
> match_key( _Key, [] , List) ->
>  List ;
> 
> match_key( Key, [Head | Tail], List ) ->
>  case element(1, Head) of
>    Key -> match_key(Key, Tail, [element(2,Head) | List] );
>    _ -> match_key(Key, Tail,  List )
>   end.

This is

match_key(Key, Db1, Db2) ->
    lists:reverse(
       [element(2,X) || X <- Db1, if element(1,X) =:= Key -> true ; _ -> false end])
    ++ Db2.

and it is difficult to believe that the reversal is desired.
> 
> match( Key, [] ) ->
>  {error, "Instance"};
> 
> match(Key, List) ->
>  match_key (Key, List, []).


Once again, this will report a missing key if and only if there are
NO keys.

match(Key, Db) ->
    match(Key, Db, not_found).

match(Key, [{Key,Value}|Db], _) ->
    [Value | match(Key, Db, found)];
match(Key, [{_,_}|Db], Found) ->
    match(Key, Db, Found);
match(_Key, [], found) ->
    [];
match(_Key, [], not_found) ->
    {error,"Instance"}.

HOWEVER, reporting a missing key AT ALL in this function, EVER,
seems like a bad idea.  Why?  Because if we care, we can *tell*
because the answer is an empty list, but if we *don't* care,
we have to work extra hard to cope.

In the same way, I don't really see the point in complaining
if you are asked to delete a key and it isn't there.  A
data base without that particular key?   Isn't that what you
_want_ when you call delete/2?
> 
> 
> Now I have 2 questions :
> 
> 1) Is this a good solution.

No.

> 2) Can I somewhere hide my helper functions (match_key, delete_key) and do not get a warning about unused.

Yes.

You "hide" a helper function by NOT EXPORTING IT.
That's all.

The way to not get a warning about a function being unused is
to USE IT.  Or if you don't have a use for it, remove it.

If the Erlang compiler tells you a function is unused, then
either
 (1) the place or places where it *is* used has or have been
     obliterated by syntax errors and if you fix those the
     compiler will notice the uses, or
 (2) the compiler is RIGHT.