[erlang-questions] Code critique please

[Anders Dahlin]

The nested case is not needed if you use a guard for the TTL. I would
probably write something like the below. Changes besides removing the
nested case are mostly for readability:

- Pick out multiple parts from the record at once, you could split this
in to the things you need for the case (Domain and TTL) and the other
later, but I don't think it adds to the readability of the code

- Key construction is less obfuscated by record operations to make it
clearer what the key is.

- Usage of traditional comment levels with two '%%'s indentation
following the code (I know erlware has a switch for this and the erlang
mode that is part of otp only has that behaviour)

- Also changed the return value from is_subscribed to {true, S} | false.
As has already been mentioned.



%% test to see if a dns_rr.domain is subscribed to
is_subscribed(_, []) ->
    false;
is_subscribed(Dom, [S| Rest]) ->
    case lists:suffix(S, Dom) of
        true  -> {true, S};
        false -> is_subscribed(Dom, Rest)
    end.

%% process the list of resource records one at a time
process_dnsrec1(Sub, []) ->
    Sub;
process_dnsrec1(Sub, [Response| Rest]) ->
    #dns_rr{domain = Domain, type = Type, class = Class, ttl = TTL} =
Response,
    case is_subscribed(Domain, dict:fetch_keys(Sub)) of
	{true, SD} when TTL =:= 0 ->
	    %% ttl == Zero, forget about the details for that server
	    NewSub = dict:store(SD, dict:new(), Sub),
	    process_dnsrec1(NewSub, []);
	{true, SD} ->
	    %% update the dns_rr to the current timestamp
	    {ok, Value} = dict:find(SD, Sub),
	    Key         = {Domain, Type, Class},
	    NewRR       = Response#dns_rr{tm = get_timestamp()},
	    NewValue    = dict:store(Key, NewRR,    Value),
	    NewSub      = dict:store(SD,  NewValue, Sub),
	    process_dnsrec1(NewSub, Rest);
	false ->
	    process_dnsrec1(Sub, Rest)
    end.


Jarrod Roberson wrote:
> is there anything that I can do to improve this code? Are the nested case of
> the best way to do that? It was the only thing I could get to work.
> 
> % test to see if a dns_rr.domain is subscribed to
> is_subscribed(_,[]) -> false;
> is_subscribed(Dom,[S|Rest]) ->
>     case lists:suffix(S,Dom) of
>         true ->
>             {ok,S};
>         false ->
>             is_subscribed(Dom,Rest)
>     end.
> 
> % process the list of resource records one at a time
> process_dnsrec1(Sub,[]) -> Sub;
> process_dnsrec1(Sub,[Response|Rest]) ->
>   Dom = Response#dns_rr.domain,
>   Key = {Response#dns_rr.domain,Response#dns_rr.type,Response#dns_rr.class},
>   case is_subscribed(Dom,dict:fetch_keys(Sub)) of
> 	  {ok,SD} ->
> 		  {ok,Value} = dict:find(SD,Sub),
>           % if the ttl == Zero then we forget about the details for that server
>           case Response#dns_rr.ttl == 0 of
>               true ->
>                   NewSub = dict:store(SD,dict:new(),Sub),
>                   process_dnsrec1(NewSub,[]);
>               false ->
>                   % update the dns_rr to the current timestamp
>                   NewRR = Response#dns_rr{tm=get_timestamp()},
>                   NewValue = dict:store(Key,NewRR,Value),
>                   NewSub = dict:store(SD,NewValue,Sub),
>                   process_dnsrec1(NewSub,Rest)
>           end;
>      false ->
>           process_dnsrec1(Sub,Rest)
>   end.
> 
> the entire source can be seen here ->
> http://github.com/fuzzylollipop/inet_mdns/blob/master/src/inet_mdns.erl
>