Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new to erlang (2 days to be honest) and I would highly appreciate some peer review. I am doing these exercises and was a bit stuck at this point:

2) Write a function which starts N processes in a ring, and sends a message M times around all the processes in the ring. After the messages have been sent the processes should terminate gracefully.

With some help from SO, I came up with a working solution, but I am not sure about its quality in any aspect: Language comprehension, Coding style, Efficiency, etc.

I will be grateful for any comment you have.

-module (concur).
-export ( [pingCircle/3, pingCircle/2] ).

pingCircle (Names, Message, TTL) ->
    Processes = lists:map (fun (Name) -> spawn (?MODULE, pingCircle, [Name, nobody] ) end, Names),
    ProcessPairs = lists:zip (Processes, rot1 (Processes) ),
    lists:map (fun ( {Process, Recipient} ) -> Process ! {setRecipient, Recipient} end, ProcessPairs),
    Circle = lists:map (fun ( {Process, _} ) -> Process end, ProcessPairs),
    hd (Circle) ! {Message, TTL - 1, lists:last (Circle) }.

rot1 ( [] ) -> [];
rot1 ( [Head | Tail] ) -> Tail ++ [Head].

pingCircle (Name, Recipient) ->
    receive
        {setRecipient, NewRecipient} ->
            pingCircle (Name, NewRecipient);
        {Message, 0, Originator} ->
            io:format ("~s received ~p with TTL 0 and dying.~n", [Name, Message] ),
            if
                Originator == self () -> io:format ("All dead.~n");
                true -> Recipient ! {Message, 0, Originator}
            end;
        {Message, TTL, Originator} ->
            io:format ("~s received ~p with TTL ~p.~n", [Name, Message, TTL] ),
            if
                Originator == self () -> Recipient ! {Message, TTL - 1, Originator};
                true -> Recipient ! {Message, TTL, Originator}
            end,
            pingCircle (Name, Recipient)
    end.
share|improve this question
add comment

3 Answers

lists:map (fun ( {Process, Recipient} ) -> Process ! {setRecipient, Recipient} end, ProcessPairs)

Whenever you use lists:map and don't use its return values, you almost certainly want to be using lists:foreach instead. map will not only build up a list that you never use, it also does not guarantee that the elements will be processed in any specific order.


Circle = lists:map (fun ( {Process, _} ) -> Process end, ProcessPairs),

Unless I'm missing something here, the list you'll get back from map will be the same as your original Processes list, which you built ProcessPairs from. So you can just use Processes and get rid of Circle.

share|improve this answer
add comment

You can replace

spawn (?MODULE, pingCircle, [Name, nobody] )

with

spawn (fun() -> pingCircle(Name, nobody) end)

This action allows do not export pingCircle/2 from the module. It is also recommended to use spawn_link instead of spawn, because it helps to avoid lost processes.

share|improve this answer
add comment

I'm reading Erlang Programming by Cesarini and Thompson, which poses essentially the same question. I tried it a couple different ways, and ended up with the code below. Essentially it creates the ring from tail (self()) to head (FirstNode) then injects user messages into FirstNode, followed by a quit message. The test() function creates a ring of a million threads and sends a thousand messages.

Edit:

Modified to wait before sending the quit, and display a count of messages that passed the master node. Note that in Erlang Programming, the problem doesn't specify that messages should stop at the originator, nor that messages start at any nodes other than the first node.

-module(ring2).
-export([test/0, test_race/0, start/3, node_loop/2, fire_quit_later/1]).

test() -> ring2:start(2, 1000000, "Hey").

start(NumMsgs, NumProcs, Message) ->
  FirstNode = send_messages(create_node(NumProcs, self()), NumMsgs, Message),
  spawn(ring2, fire_quit_later, [FirstNode]),
  MessageCount = node_loop(FirstNode, 0),
  io:format("Done at ~p. Master node processed ~p messages.~n", [calendar:local_time(), MessageCount]).

fire_quit_later(FirstNode) ->
  receive
  after 20000 -> io:format("Firing quit at ~p~n", [calendar:local_time()]), FirstNode ! quit
  end.

send_messages(FirstNode, 0, _) -> FirstNode;
send_messages(FirstNode, NumMsgs, Message) ->
  FirstNode ! Message,
  send_messages(FirstNode, NumMsgs - 1, Message).

create_node(0, NextNode) -> NextNode;
create_node(NodeNumber, NextNode) -> create_node(NodeNumber - 1, spawn(ring2, node_loop, [NextNode, 0])).

node_loop(NextNode, MessageCount) ->
  receive
    quit -> NextNode ! quit, MessageCount;
    Message -> NextNode ! Message, node_loop(NextNode, MessageCount + 1)
  end.

test_race() ->
  self() ! quit,
  receive
    quit -> done
  after 1000 -> {error, no_message}
  end.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.