-
-
Notifications
You must be signed in to change notification settings - Fork 398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refine Take-A-Number #1511
Refine Take-A-Number #1511
Conversation
Thank you for contributing to Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:
Automated comment created by PR Commenter 🤖. |
- You will need a new named function. You can name it, for example, `loop`. | ||
- This new function should accept one argument, the state. | ||
- The 0-arity anonymous function from the previous step can call the new named function, passing the initial state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope those instructions are explicit enough to lead people to a solution similar to the exemplar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they should be
@@ -15,12 +15,15 @@ Note that each time you run this code, the PID may be different. | |||
|
|||
## 2. Report the machine state | |||
|
|||
Modify the machine so that it can receive `{:report_state, sender_pid}` messages. It should send its current state (the last given out ticket number) to `sender_pid` and then wait for more messages. | |||
Modify the machine so that the newly spawned process is ready to receive messages (start a _receive loop_) with an initial state of `0`. It should be able to receive `{:report_state, sender_pid}` messages. As a response to those messages, it should send its current state (the last given out ticket number) to `sender_pid` and then wait for more messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that the phrase "receive loop" will remind people of the paragraph with the same title from the introductions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered adding some example code in the receive loop section? I think code will make it a lot more memorable. Something like:
def loop(ping_count) do
receive do
{:ping, sender_pid} ->
send(sender_pid, :pong)
loop(ping_count + 1)
end
end
It's pretty close to the solution though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, many times, but it is clearly against the recommendation:
Code examples should only be used to introduce new syntax (students should not need to search the web for examples of syntax). In other cases provide descriptions or links instead of code.
Defining a recursive function is not new syntax...
|
||
# a client sending a message to the machine | ||
send(machine_pid, {:report_state, self()}) | ||
|
||
# a client receiving a message from the machine | ||
receive do | ||
msg -> msg | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those comments helpful? I was staring at this snippet and I was afraid that people might not understand that this code is an example code of how to test the implementation of the machine, and not the code that should go into the implementation of the machine 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your concern, but that's usually what those code snippets do.
Maybe if you wanted to be even more explicit, you could frame those as tests?
test "reports its own state" do
pid = TakeANumber.start()
send(pid, {:report_state, self()})
assert_receive 0
end
but that might not help so much.
I think adding the comments is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a nice improvement.
Sorry I didn't look too closely at #1486.
@@ -15,12 +15,15 @@ Note that each time you run this code, the PID may be different. | |||
|
|||
## 2. Report the machine state | |||
|
|||
Modify the machine so that it can receive `{:report_state, sender_pid}` messages. It should send its current state (the last given out ticket number) to `sender_pid` and then wait for more messages. | |||
Modify the machine so that the newly spawned process is ready to receive messages (start a _receive loop_) with an initial state of `0`. It should be able to receive `{:report_state, sender_pid}` messages. As a response to those messages, it should send its current state (the last given out ticket number) to `sender_pid` and then wait for more messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered adding some example code in the receive loop section? I think code will make it a lot more memorable. Something like:
def loop(ping_count) do
receive do
{:ping, sender_pid} ->
send(sender_pid, :pong)
loop(ping_count + 1)
end
end
It's pretty close to the solution though.
|
||
# a client sending a message to the machine | ||
send(machine_pid, {:report_state, self()}) | ||
|
||
# a client receiving a message from the machine | ||
receive do | ||
msg -> msg | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your concern, but that's usually what those code snippets do.
Maybe if you wanted to be even more explicit, you could frame those as tests?
test "reports its own state" do
pid = TakeANumber.start()
send(pid, {:report_state, self()})
assert_receive 0
end
but that might not help so much.
I think adding the comments is fine for now.
- You will need a new named function. You can name it, for example, `loop`. | ||
- This new function should accept one argument, the state. | ||
- The 0-arity anonymous function from the previous step can call the new named function, passing the initial state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they should be
Resolves #1482
This PR reverts #1486 - see my new comment in that PR to know why.
This PR re-distributes work between step 1 and step 2. Previously, step 1 assumed you would already start the receive loop. That doesn't make sense because there is no unit test for step 1 that could test that you have done this correctly. So I'm moving that to step 2.