<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jan 31, 2015 at 9:31 AM, Roelof Wobben <span dir="ltr"><<a href="mailto:r.wobben@home.nl" target="_blank">r.wobben@home.nl</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Thanks,<br>
<br>
Everything is working now.<br>
Finnaly I have solved the whole exercise like this :<br>
<br>
-module(db).<br>
<br>
-export([new/0, destroy/1, write/3, read/2, delete_key/3, delete/2, match/2, match_key/3]).<br>
<br>
new() -><br>
[].<br>
<br>
destroy(_Db) -><br>
ok.<br>
<br>
write(Key, Element, Db) -><br>
[ {Key, Element} | Db ].<br>
<br>
read(_Key, []) -><br>
{error, "Instance"};<br>
<br>
read(Key, [Head | List]) -><br>
case element(1, Head) of<br>
Key -> {ok, element(2, Head)};<br>
_ -> read(Key, List)<br>
end.<br>
<br>
delete_key( _Key, [] , List2) -><br>
List2 ;<br>
<br>
delete_key( Key, [Head | Tail], List3 ) -><br>
case element(1, Head) of<br>
Key -> delete_key(Key, Tail, List3 );<br>
_ -> delete_key(Key, Tail, [Head | List3] )<br>
end.<br>
<br>
delete( _Key, [] ) -><br>
{error, "Instance"};<br>
<br>
delete(Key, List) -><br>
match_key (Key, List, []).<span class=""><br>
<br>
match_key( _Key, [] , List) -><br>
List ;<br>
<br>
match_key( Key, [Head | Tail], List ) -><br>
case element(1, Head) of<br>
Key -> match_key(Key, Tail, [element(2,Head) | List] );<br>
_ -> match_key(Key, Tail, List )<br>
end.<br>
<br></span><span class="">
match( Key, [] ) -><br>
{error, "Instance"};<br>
<br>
match(Key, List) -><br></span>
match_key (Key, List, []).<br>
<br>
<br>
Now I have 2 questions :<br>
<br>
1) Is this a good solution.<br></blockquote><div><br></div><div>Does it work? If it works, then it's potentially a good solution. But you should think about including test code so you can refactor with confidence, and to make it easier for others to provide the help you're seeking.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
2) Can I somewhere hide my helper functions (match_key, delete_key) and do not get a warning about unused.</blockquote><div><br></div><div>If you're getting warnings about unused functions, there's a chance the way you're calling them doesn't match their definitions. Assuming you're working from example 3-4 of "Erlang Programming", it wants new/0, destroy/1, write/3, delete/2, read/2, and match/2 to comprise the API. So let's make that change:</div><div><br></div><div><div>-export([new/0, destroy/1, write/3, delete/2, read/2, match/2]).</div></div><div><br></div><div>Compiling your code with this change yields:</div><div><br></div><div><div>/private/tmp/db.erl:23: Warning: function delete_key/3 is unused</div><div>/private/tmp/db.erl:47: Warning: variable 'Key' is unused</div></div><div><br></div><div>Why is this delete_key/3 unused? The reason is that the only place that calls it is the delete_key/3 function itself. If it's not exported, then only functions in the same module can call it, and of no other functions in the module call it, it's unused. As pointed out previously, the bug is here:</div><div><br></div><div><div>delete(Key, List) -></div><div> match_key (Key, List, []).</div></div><div><br></div><div>which should be:</div><div><br></div><div><div>delete(Key, List) -></div><div> delete_key (Key, List, []).</div></div><div><br></div><div>If we make that change, and add a leading underscore to Key on line 47, the code compiles without warning. But does it work? Let's add an exported test/0 function based on the sample session in the book to see:</div><div><br></div><div><div>test() -></div><div> Db = db:new(),</div><div> Db1 = db:write(francesco, london, Db),</div><div> Db2 = db:write(lelle, stockholm, Db1),</div><div> {ok, london} = db:read(francesco, Db2),</div><div> Db3 = db:write(joern, stockholm, Db2),</div><div> {error, instance} = db:read(ola, Db3),</div><div> [joern, lelle] = db:match(stockholm, Db3),</div><div> Db4 = db:delete(lelle, Db3),</div><div> [joern] = db:match(stockholm, Db4),</div></div><div> ok.</div><div><br></div><div>Then run it:</div><div><br></div><div><div>5> db:test().</div><div>** exception error: no match of right hand side value {error,"Instance"}</div><div> in function db:test/0 (/private/tmp/db.erl, line 59)</div></div><div><br></div><div>Line 59 is:</div><div><br></div><div><div> {error, instance} = db:read(ola, Db3),</div></div><div><br></div><div>The error is a failure to match the return value of read/2 against the expected {error, instance}. The problem is here:</div><div><br></div><div><div>read(_Key, []) -></div><div> {error, "Instance"};</div></div><div><br></div><div>Let's change that:</div><div><br></div><div><div>read(_Key, []) -></div><div> {error, instance};</div></div><div><br></div><div>Also change the failing clauses for match/2 and delete/2 the same way. Now recompile and rerun the test:</div><div><br></div><div><div>7> db:test().</div><div>** exception error: no match of right hand side value []</div><div> in function db:test/0 (/private/tmp/db.erl, line 60)</div></div><div><br></div><div>Line 60 is:</div><div><br></div><div><div> [joern, lelle] = db:match(stockholm, Db3),</div></div><div><br></div><div>Looks like match/2 doesn't work, since it's returning an empty list. Why? match/2 just calls match_key/3:</div><div><br></div><div><div>match_key( _Key, [] , List) -></div><div> List ;</div><div><br></div><div>match_key( Key, [Head | Tail], List ) -></div><div> case element(1, Head) of</div><div> Key -> match_key(Key, Tail, [element(2,Head) | List] );</div><div> _ -> match_key(Key, Tail, List )</div><div> end.</div></div><div><br></div><div>The problem is the call to element(1, Head) returns the Name element of the tuple, not the Location. You have your calls to element(1, Head) and element(2, Head) in the wrong places in this function. Let's swap them:</div><div><br></div><div><div>match_key( Key, [Head | Tail], List ) -></div><div> case element(2, Head) of</div><div> Key -> match_key(Key, Tail, [element(1,Head) | List] );</div><div> _ -> match_key(Key, Tail, List )</div><div> end.</div></div><div><br></div><div>Recompile and re-test:</div><div><br></div><div><div>9> db:test().</div><div>** exception error: no match of right hand side value [lelle,joern]</div><div> in function db:test/0 (/private/tmp/db.erl, line 60)</div></div><div><br></div><div>The test expects [joern,lelle] but match/2 now returns [lelle,joern]. Why? The reason is here, on line 43:</div><div><br></div><div><div> Key -> match_key(Key, Tail, [element(1,Head) | List] );</div></div><div><br></div><div>Every time we find a match, we push the result onto the head of our accumulator list passed to the next iteration. That means the last result we find will be first in the results. This is a common issue with accumulator-based recursive functions, and normally you'd fix it by calling lists:reverse/1 to reverse the final result at line 39:</div><div><br></div><div><div>match_key( _Key, [] , List) -></div><div> lists:reverse(List);</div></div><div><br></div><div>But the book says no calls to the lists module are allowed, so we can't do this. Another approach is to append to the accumulator list instead, at line 43:</div><div><br></div><div><div> Key -> match_key(Key, Tail, List ++ [element(1,Head)]);</div></div><div><br></div><div>With this change in place, let's recompile and rerun the tests:</div><div><br></div><div><div>11> db:test().</div><div>ok</div></div><div><br></div><div>Yay, the test works. Now, can we improve the code?</div><div><br></div><div>One thing that sticks out are the calls to element/2. Those can be replaced with tuple pattern matching. For example, read/2 looks like this:</div><div><br></div><div><div>read(Key, [Head | List]) -></div><div> case element(1, Head) of</div><div> Key -> {ok, element(2, Head)};</div><div> _ -> read(Key, List)</div><div> end.</div></div><div><br></div><div>Let's get rid of those element/2 calls. The case is simply doing a match of the first element of each tuple against Key. We can do that right in the function head, like this:</div><div><br></div><div><div>read(Key, [{Key,Location} | List]) -></div><div> {ok, element(2, Head)};<br></div><div><br></div></div><div>And since element(2,Head) is just Location, we can just make it this:</div><div><br></div><div><div>read(Key, [{Key,Location} | _List]) -></div><div> {ok, Location};<br></div></div><div><br></div><div>OK, that works if Key matches the name in the head tuple. But what if Key doesn't match? We need another read/2 clause, originally handled by the second clause of the case statement:</div><div><br></div><div><div>read(Key, [_Head | List]) -></div><div> read(Key, List).</div></div><div><br></div><div>With these changes in place, do our tests still pass?</div><div><br></div><div><div>16> db:test().</div><div>ok</div></div><div><br></div><div>Yep. Now, where else do we call element/2? In delete_key/3:</div><div><br></div><div><div>delete_key( Key, [Head | Tail], List3 ) -></div><div> case element(1, Head) of</div><div> Key -> delete_key(Key, Tail, List3 );</div><div> _ -> delete_key(Key, Tail, [Head | List3] )</div><div> end.</div></div><div><br></div><div>We can change this roughly the same way we changed read/2, like this:</div><div><br></div><div><div>delete_key(Key, [{Key,_Location} | Tail], List3) -></div><div> delete_key(Key, Tail, List3);</div><div>delete_key(Key, [Head | Tail], List3) -></div><div> delete_key(Key, Tail, [Head | List3]).</div></div><div><br></div><div>and rerun the tests:</div><div><br></div><div><div>18> db:test().</div><div>ok</div></div><div><br></div><div>BTW, notice that delete_key/3 also reverses the result list. For consistency, change it to use append instead, like we did earlier for match_key/3:</div><div><br></div><div><div>delete_key(Key, [{Key,_Location} | Tail], List3) -></div><div> delete_key(Key, Tail, List3);</div><div>delete_key(Key, [Head | Tail], List3) -></div><div> delete_key(Key, Tail, List3 ++ [Head]).</div></div><div><br></div><div>And the tests still pass. The other use of element/2 is in match_key/3, so let's fix that the same way:</div><div><br></div><div><div>match_key(Key, [{Name,Key} | Tail], List) -></div><div> match_key(Key, Tail, List ++ [Name]);</div><div>match_key(Key, [_Head | Tail], List) -></div><div> match_key(Key, Tail, List).</div></div><div><br></div><div>The difference here from previous matches is the Key matches element 2 rather than element 1. The tests still pass with this change:</div><div><br></div><div><div>22> db:test().</div><div>ok</div></div><div><br></div><div>One problem with the code is that the write/2 function unconditionally adds the name/location without checking to see if name is already there. Most people can't be in multiple locations at once, so get rid of any matching name/location first:</div><div><br></div><div><div>write(Name, Location, Db) -></div><div> NewDb = case read(Name, Db) of</div><div> {ok,_Location} -></div><div> delete(Name, Db);</div><div> {error,instance} -></div><div> Db</div><div> end,</div><div> [{Name, Location} | NewDb].</div></div><div><br></div><div>We first read to see if the name is present; if so, we delete it and used the modified db to write. If not found, we just use the db passed in to write. And the tests still pass with this change, which isn't too surprising since we're not specifically testing this case.</div><div><br></div><div>At this stage, another improvement is to rename the *_key functions -- there's no need for the _key suffix, they can instead just be named delete and match because they have arity 3, unlike their exported counterparts, which have arity 2. The arity difference prevents them from clashing. I would also stop using Key and Element as parameter names and use Name and Location instead, as they're more descriptive for the problem.</div><div><br></div><div>With these final changes in place, the whole refactored module looks like this:</div><div><br></div><div><div><div>-module(db).</div><div><br></div><div>-export([new/0, destroy/1, write/3, delete/2, read/2, match/2, test/0]).</div><div><br></div><div>new() -></div><div> [].</div><div><br></div><div>destroy(_Db) -></div><div> ok.</div><div><br></div><div>write(Name, Location, Db) -></div><div> NewDb = case read(Name, Db) of</div><div> {ok,_Location} -></div><div> delete(Name, Db);</div><div> {error,instance} -></div><div> Db</div><div> end,</div><div> [{Name, Location} | NewDb].</div><div><br></div><div>read(_Name, []) -></div><div> {error, instance};</div><div><br></div><div>read(Name, [{Name,Location} | _List]) -></div><div> {ok, Location};</div><div>read(Name, [_Head | List]) -></div><div> read(Name, List).</div><div><br></div><div>delete(_Name, [] , List) -></div><div> List;</div><div><br></div><div>delete(Name, [{Name,_Location} | Tail], List) -></div><div> delete(Name, Tail, List);</div><div>delete(Name, [Head | Tail], List) -></div><div> delete(Name, Tail, List ++ [Head]).</div><div><br></div><div>delete(_Name, []) -></div><div> {error, instance};</div><div><br></div><div>delete(Name, List) -></div><div> delete(Name, List, []).</div><div><br></div><div>match( _Name, [] , List) -></div><div> List;</div><div><br></div><div>match(Location, [{Name,Location} | Tail], List) -></div><div> match(Location, Tail, List ++ [Name]);</div><div>match(Location, [_Head | Tail], List) -></div><div> match(Location, Tail, List).</div><div><br></div><div>match(_Location, []) -></div><div> {error, instance};</div><div>match(Location, List) -></div><div> match(Location, List, []).</div><div><br></div><div>test() -></div><div> Db = db:new(),</div><div> Db1 = db:write(francesco, london, Db),</div><div> Db2 = db:write(lelle, stockholm, Db1),</div><div> {ok, london} = db:read(francesco, Db2),</div><div> Db3 = db:write(joern, stockholm, Db2),</div><div> {error, instance} = db:read(ola, Db3),</div><div> [joern, lelle] = db:match(stockholm, Db3),</div><div> Db4 = db:delete(lelle, Db3),</div><div> [joern] = db:match(stockholm, Db4),</div><div> ok.</div></div></div><div><br></div><div>Study all these changes and make sure you understand the reasoning behind them.</div><div><br></div><div>--steve</div></div></div></div>