Skip to content

Instantly share code, notes, and snippets.

@7-fl
Last active April 28, 2017 09:31
Show Gist options
  • Save 7-fl/ec51e9ed0b46e54f91d36b9035397ef2 to your computer and use it in GitHub Desktop.
Save 7-fl/ec51e9ed0b46e54f91d36b9035397ef2 to your computer and use it in GitHub Desktop.
Review of Kevin Marth's Supervisor-Frequency Server

Correctness

If I kill the supervisor using the observer app, your code will print out erroneous information. When the supervisor is killed, your code executes:

        {'EXIT',Client,_} -> 
            io:format("server: #~p client exit ~p~n",[Number,Client]), 
            server(Number,exited(Frequencies,Client)) 

but the Client variable will contain the Pid of the supervisor--not a client.


Because a case statement is an expression, it returns a value, so the following case statement:

    case Number of 
        1 -> Frequencies = [10,11,12,13,14,15]; 
        2 -> Frequencies = [20,21,22,23,24,25] 
    end, 

can also be written as:

   Frequencies = 
       case Number of 
           1 -> [10,11,12,13,14,15]; 
           2 -> [20,21,22,23,24,25] 
       end, 

Because you are using the Tuple variable as a catch all variable here:

    try Tuple = lists:keyfind(Freq,1,Allocated) of 
        Tuple -> unlink(Client), 
                 {{[Freq|Available],lists:keydelete(Freq,1,Allocated)},ok} 
    catch 
        error:{badmatch,_} -> exit(Client,kill), {Frequencies,error} 
    end.

you could write that more succinctly as:

   try lists:keyfind(Freq,1,Allocated) of 
       _ -> unlink(Client), 
                {{[Freq|Available],lists:keydelete(Freq,1,Allocated)},ok} 
   catch 
       error:{badmatch,_} -> exit(Client,kill), {Frequencies,error} 
   end.
   

Also, in the above code you call exit(Client, kill) to kill the client. But exit(Pid, kill) is used to immediately kill processes that are trapping exits, yet the client is not trapping exits. I don't know what good practice is in that regard, but you could have killed the client with something less drastic: exit(Pid, not_allocated).

Readability

For the assignments, you need to post your code off site at github, hastebin, etc., then post a link to your code for the reviewer. At hastebin, you don't even need to sign up to post your code. Making someone review unformatted code is too much of a burden.

In my opinion, writing things in this format:

catch 
    error:{badmatch,_} -> exit(Client,kill), {Frequencies,error} 
end.

makes your code harder to read. In other languages, I see people get infatuated with one liners, but in my opinion one liners are hard to decipher, and I feel code clarity should trump brevity every time. I would rather see you write:

catch 
    error:{badmatch,_} -> 
            exit(Client,kill), 
            {Frequencies,error} 
end.

If you only have one line of code after the arrow, then I think it's okay to put the line of code on the same line.

Using the Observer

See my comment under Correctness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment