Issue 4306045: code review 4306045: A codewalk through a simple program that illustrates se... (Closed)
|
Created:
14 years, 1 month ago by denero Modified:
14 years ago Reviewers:
CC:
adg, rsc, iant2, r, golang-dev Visibility:
Public. |
DescriptionA codewalk through a simple program that illustrates several aspects of Go functions: function objects, higher-order functions, variadic functions, tail recursion, etc. The example program simulates the game of Pig, a dice game with simple rules but a nontrivial solution.
Patch Set 1 #Patch Set 2 : diff -r c5c62aeb6267 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 94fab267e96b https://go.googlecode.com/hg/ #
Total comments: 84
Patch Set 4 : diff -r 7df08a2207e6 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 7df08a2207e6 https://go.googlecode.com/hg/ #
Total comments: 18
Patch Set 6 : diff -r 7df08a2207e6 https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 7 : diff -r 7df08a2207e6 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r 7df08a2207e6 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 9 : diff -r 7df08a2207e6 https://go.googlecode.com/hg/ #MessagesTotal messages: 26
Hello adg (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
FYI http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml File doc/codewalk/functions.xml (right): http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:94: The current 6g and gccgo implementations of Go do optimize tail-recursive This is not true. 6g does not optimize tail recursive calls, and I hope it will stay that way (I like stack traces).
Sign in to reply to this message.
Interesting. Ian's reply here (2nd message in thread) hinted that there was. http://groups.google.com/group/golang-nuts/browse_thread/thread/d282193c786b0... In this case, we should remove the recursive function & associated codewalk step. Shorter is better. -John On Wed, Mar 23, 2011 at 6:46 AM, <rsc@golang.org> wrote: > FYI > > > > http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml > File doc/codewalk/functions.xml (right): > > http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... > doc/codewalk/functions.xml:94: The current 6g and gccgo implementations > of Go do optimize tail-recursive > This is not true. 6g does not optimize tail recursive > calls, and I hope it will stay that way (I like stack traces). > > http://codereview.appspot.com/4306045/ >
Sign in to reply to this message.
On Wed, Mar 23, 2011 at 10:31, John DeNero <denero@google.com> wrote: > Interesting. Ian's reply here (2nd message in thread) hinted that there was. I think you've found one of the rare instances of Ian being wrong. :-) The linkers that 6g etc use once supported a language called Alef that had an explicit "become" statement that demanded a tail call optimization, and that gets mentioned once in a while to suggest that 6g might use it eventually for tail calls, but I don't think it will happen. We like useful stack traces too much. Also, a function with a defer or perhaps other automatic cleanup would not be able to do a tail call optimization, so it definitely isn't something you could rely on in general, even in gccgo. Russ
Sign in to reply to this message.
Russ Cox <rsc@golang.org> writes: > On Wed, Mar 23, 2011 at 10:31, John DeNero <denero@google.com> wrote: >> Interesting. Ian's reply here (2nd message in thread) hinted that there was. > > I think you've found one of the rare instances of Ian being wrong. :-) I am often wrong. Unfortunately I don't remember what led me to be wrong in that way. Oh well. > The linkers that 6g etc use once supported a language called > Alef that had an explicit "become" statement that demanded > a tail call optimization, and that gets mentioned once in > a while to suggest that 6g might use it eventually for tail > calls, but I don't think it will happen. We like useful stack traces > too much. Also, a function with a defer or perhaps other > automatic cleanup would not be able to do a tail call optimization, > so it definitely isn't something you could rely on in general, > even in gccgo. Indeed. For the record, for people who like stack traces, you can disable tail calls in gccgo using -fno-optimize-sibling-calls. Ian
Sign in to reply to this message.
Thanks very much for writing this. It's a nice example that covers some interesting ground. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml File doc/codewalk/functions.xml (right): http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:21: opponent. Any other roll adds to your turn points the value of the roll. </li> "Any other roll adds its value to your turn points." http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:22: <li> If you stay, you bank your turn points and play passes to your opponent. </li> "If you stay, your turn points are added to your total and play passes to your opponent." http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:28: A state in the game (type pigState) includes points scored in previous turns The <code>pigState</code> type stores the points of the current and opposing players, and the points accumulated during the current turn. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:33: Go allows user-defined types to be functions, which must specify input and In Go, functions can be passed around just like any other value. A function's type signature describes the types of its arguments and return values. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:37: In Pig, an action maps from a pigState to a pair of outputs, a successor state The <code>action</code> type is a function that takes a <code>pigState</code> and returns the resulting <code>pigState</code> and whether the current turn is over. If the turn is over the <code>pigState</code>'s <code>playerScore</code> and <code>opponentScore</code> fields should be swapped, as it is now the other player's turn. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:43: One of Go's unusual features is that functions and methods can return multiple Go functions can return multiple values. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:44: values. Returned values are ordered. A Go idiom is to return a primary value Drop these next two sentences. There's no context to them here. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:48: In Pig, the two actions roll and stay satisfy the action type signature by s/In Pig, the two actions/The functions/ put <code> around roll, stay, and action s/satisfy/match/ s/ by/./ http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:49: each returning a pair of values. These actions encapsulate the rules of Pig These <code>action</code> functions describe the rules of Pig. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:54: Functions can take other functions as arguments and return functions as A function can use other functions as arguments and return values. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:58: Pig represents a policy (or strategy) as a function from pigStates to actions, A <code>policy</code> (or strategy) is a function that takes a <code>pigState</code>, determines its next move, and returns the appropriate <code>action</code>. (Remember, an <code>action</code> is itself a function.) http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:69: least k points in a turn, and then stay. The argument k is preserved in the The argument <code>k</code> is enclosed by the function literal returned here. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:75: player reaches 100 (pigGoal) points. Actions are selected by calling the Actions are selected by calling the policy function associated with the current player. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:85: In Pig, we enforce that policies only return one of the two legal actions in the game, roll or stay. s/In Pig, w/W/ s/policies/a policy function/ s/return/returns/ http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:88: <step title="Recursive functions and optimization" drop this section. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:105: <step title="Variadic function declarations" src="doc/codewalk/pig.go:/\/\/ ratios/,/string {/"> s/ratios/ratioS/ - it's currently broken. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:113: ellipsis syntax here. Perhaps describe this specific example more completely. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode14 doc/codewalk/pig.go:14: pigGoal = flag.Int("pigGoal", 100, "Winning score in the game of Pig") I'm in two minds as to whether these should be flags or just const values. They're not discussed in the codewalk, so it might be clearer if they were just consts. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode19 doc/codewalk/pig.go:19: // as well as the points scored by player in the current turn. s/the current/this/ s/by/by the current/ http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode24 doc/codewalk/pig.go:24: // An action simulates a state transition in a Pig game. "simulates" doesn't seem like the right word here, but I can't think of a better replacement. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode27 doc/codewalk/pig.go:27: // roll returns the (successorState, turnIsOver) result of simulating a die roll. This comment should describe what actually happens and why. In particular, it would have been easier for me to understand were the swapping of the player and opponent scores highlighted. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode36 doc/codewalk/pig.go:36: // stay returns the (successorState, turnIsOver) result of staying. ditto my comment for roll http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode41 doc/codewalk/pig.go:41: // A policy (or player strategy) is a function from states to actions How about: "A policy (or player strategy) is a function that chooses an action for a given pigState." http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode44 doc/codewalk/pig.go:44: // The stayAtKPolicy rolls until k turnPoints are earned and then stays. s/The // http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode56 doc/codewalk/pig.go:56: nextPlayer := rand.Intn(2) // Randomly decide who plays first move this line below the "state :=" line maybe it should be currentPlayer ? http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode73 doc/codewalk/pig.go:73: // playPigRecursive simulates a Pig game with a tail-recursive function. As discussed, this function and its associated commentary should be dropped. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode88 doc/codewalk/pig.go:88: func roundRobin(game func(policy, policy) int, policies []policy) ([]int, []int) { It seems an unnecessary complexity for roundRobin to take a game parameter. Just hard-code the call to playPig instead. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode92 doc/codewalk/pig.go:92: matchWins := make([]int, 2) // matchWins[0] holds win count for i s/match/pair/ ? The word "match" is overloaded in English. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode94 doc/codewalk/pig.go:94: matchWins[game(policies[i], policies[j])]++ perhaps split this into two lines: winner := game(policies[i], policies[j]) matchWins[winner]++ http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode105 doc/codewalk/pig.go:105: // ratioString joins each element annotated by its fraction of the total. I know "join" is the common term in functional programming, but it's not really what's happening here. Perhaps something like: ratioString takes a list of integer values and returns a string that lists each value and its percentage of the sum of all values. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode123 doc/codewalk/pig.go:123: for k := 0; k < *pigGoal; k++ { for k := range policies { http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode128 doc/codewalk/pig.go:128: for k := 0; k < len(policies); k++ { for k := range policies {
Sign in to reply to this message.
http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml File doc/codewalk/functions.xml (right): http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:37: In Pig, an action maps from a pigState to a pair of outputs, a successor state s/,/:/ to make clear that what follows is not part of a list http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:38: and a bit that indicates whether the current turn has completed, which swaps s/bit/boolean/ http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:44: values. Returned values are ordered. A Go idiom is to return a primary value I'd say ... to return the main values first and an error code, if any, last. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode14 doc/codewalk/pig.go:14: pigGoal = flag.Int("pigGoal", 100, "Winning score in the game of Pig") You can remove the word pig from essentially every name where it appears in this program. Here -goal is a nicer flag anyway (if you keep the flag). Similarly gamesPerMatch -> games. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode25 doc/codewalk/pig.go:25: type action func(currentState pigState) (nextState pigState, turnIsOver bool) type action func(current pigState) (next pigState, turnIsOver bool) http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode45 doc/codewalk/pig.go:45: func stayAtKPolicy(k int) policy { It's a little confusing that stayAtKPolicy is not a policy. I think it would be fine to call it just stayAtK. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode87 doc/codewalk/pig.go:87: // The total number of wins, losses for policy i are returned in wins[i], losses[i] Parallel arrays are almost always a sign that a you should be using a real data structure (or that you are a Fortran programmer). wins[i] + losses[i] == a constant, right? So maybe one can be dropped? http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode112 doc/codewalk/pig.go:112: var fmtString string This is a somewhat convoluted way to use Sprintf. It would be simpler to accumulate the actual string: s := "" for i, val := range vals { if i > 0 { s += ", " } pct := 100*float64(val)/float64(total) s += fmt.Sprintf("%d/%d (%0.1f%%)", val, total, pct) } return s
Sign in to reply to this message.
Hello adg, rsc, iant2 (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thanks very much for all the comments. The codewalk is cleaner and shorter now, and I learned a bit about Go along the way. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml File doc/codewalk/functions.xml (right): http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:13: called <a href="http://cs.gettysburg.edu/projects/pig/">Pig</a> and evaluates Now links to wikipedia. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:21: opponent. Any other roll adds to your turn points the value of the roll. </li> On 2011/03/28 03:25:26, adg wrote: > "Any other roll adds its value to your turn points." Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:22: <li> If you stay, you bank your turn points and play passes to your opponent. </li> On 2011/03/28 03:25:26, adg wrote: > "If you stay, your turn points are added to your total and play passes to your > opponent." Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:28: A state in the game (type pigState) includes points scored in previous turns On 2011/03/28 03:25:26, adg wrote: > The <code>pigState</code> type stores the points of the current and opposing > players, and the points accumulated during the current turn. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:33: Go allows user-defined types to be functions, which must specify input and On 2011/03/28 03:25:26, adg wrote: > In Go, functions can be passed around just like any other value. A function's > type signature describes the types of its arguments and return values. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:37: In Pig, an action maps from a pigState to a pair of outputs, a successor state On 2011/03/28 03:25:26, adg wrote: > The <code>action</code> type is a function that takes a <code>pigState</code> > and returns the resulting <code>pigState</code> and whether the current turn is > over. > > If the turn is over the <code>pigState</code>'s <code>playerScore</code> and > <code>opponentScore</code> fields should be swapped, as it is now the other > player's turn. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:38: and a bit that indicates whether the current turn has completed, which swaps On 2011/03/28 03:35:01, rsc wrote: > s/bit/boolean/ Adopted adg's change above. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:43: One of Go's unusual features is that functions and methods can return multiple On 2011/03/28 03:25:26, adg wrote: > Go functions can return multiple values. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:44: values. Returned values are ordered. A Go idiom is to return a primary value On 2011/03/28 03:25:26, adg wrote: > Drop these next two sentences. There's no context to them here. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:44: values. Returned values are ordered. A Go idiom is to return a primary value On 2011/03/28 03:35:01, rsc wrote: > I'd say ... to return the main values first and > an error code, if any, last. Dropped. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:48: In Pig, the two actions roll and stay satisfy the action type signature by On 2011/03/28 03:25:26, adg wrote: > s/In Pig, the two actions/The functions/ > put <code> around roll, stay, and action > s/satisfy/match/ > s/ by/./ Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:49: each returning a pair of values. These actions encapsulate the rules of Pig On 2011/03/28 03:25:26, adg wrote: > These <code>action</code> functions describe the rules of Pig. Done, except s/describe/define. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:54: Functions can take other functions as arguments and return functions as On 2011/03/28 03:25:26, adg wrote: > A function can use other functions as arguments and return values. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:58: Pig represents a policy (or strategy) as a function from pigStates to actions, On 2011/03/28 03:25:26, adg wrote: > A <code>policy</code> (or strategy) is a function that takes a > <code>pigState</code>, determines its next move, and returns the appropriate > <code>action</code>. (Remember, an <code>action</code> is itself a function.) Done, with slight rewording. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:69: least k points in a turn, and then stay. The argument k is preserved in the On 2011/03/28 03:25:26, adg wrote: > The argument <code>k</code> is enclosed by the function literal returned here. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:70: closure returned here. Added that the function literal is a <code>policy</code> http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:75: player reaches 100 (pigGoal) points. Actions are selected by calling the On 2011/03/28 03:25:26, adg wrote: > Actions are selected by calling the policy function associated with the current > player. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:85: In Pig, we enforce that policies only return one of the two legal actions in the game, roll or stay. On 2011/03/28 03:25:26, adg wrote: > s/In Pig, w/W/ > s/policies/a policy function/ > s/return/returns/ Done. Also added <code> tags. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:88: <step title="Recursive functions and optimization" On 2011/03/28 03:25:26, adg wrote: > drop this section. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:94: The current 6g and gccgo implementations of Go do optimize tail-recursive On 2011/03/23 13:46:03, rsc wrote: > This is not true. 6g does not optimize tail recursive > calls, and I hope it will stay that way (I like stack traces). Dropped. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:105: <step title="Variadic function declarations" src="doc/codewalk/pig.go:/\/\/ ratios/,/string {/"> On 2011/03/28 03:25:26, adg wrote: > s/ratios/ratioS/ - it's currently broken. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:113: ellipsis syntax here. On 2011/03/28 03:25:26, adg wrote: > Perhaps describe this specific example more completely. Step dropped, as the associated code was convoluted. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/functions.xml#ne... doc/codewalk/functions.xml:123: optimal policy for Pig Replaced link with a google search -- more robust to future change. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode14 doc/codewalk/pig.go:14: pigGoal = flag.Int("pigGoal", 100, "Winning score in the game of Pig") On 2011/03/28 03:25:26, adg wrote: > I'm in two minds as to whether these should be flags or just const values. > They're not discussed in the codewalk, so it might be clearer if they were just > consts. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode14 doc/codewalk/pig.go:14: pigGoal = flag.Int("pigGoal", 100, "Winning score in the game of Pig") On 2011/03/28 03:35:01, rsc wrote: > You can remove the word pig from essentially every > name where it appears in this program. Here -goal is a > nicer flag anyway (if you keep the flag). Superfluous "pig"s removed. > > Similarly gamesPerMatch -> games. gamesPerMatch (now gamesPerSeries) disambiguates from gamesPerPolicy below. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode19 doc/codewalk/pig.go:19: // as well as the points scored by player in the current turn. On 2011/03/28 03:25:26, adg wrote: > s/the current/this/ > s/by/by the current/ Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode24 doc/codewalk/pig.go:24: // An action simulates a state transition in a Pig game. On 2011/03/28 03:25:26, adg wrote: > "simulates" doesn't seem like the right word here, but I can't think of a better > replacement. Changed to "An action transitions..." http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode25 doc/codewalk/pig.go:25: type action func(currentState pigState) (nextState pigState, turnIsOver bool) On 2011/03/28 03:35:01, rsc wrote: > type action func(current pigState) (next pigState, turnIsOver bool) Done. Also, I s/next/successor for consistency. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode27 doc/codewalk/pig.go:27: // roll returns the (successorState, turnIsOver) result of simulating a die roll. On 2011/03/28 03:25:26, adg wrote: > This comment should describe what actually happens and why. > > In particular, it would have been easier for me to understand were the swapping > of the player and opponent scores highlighted. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode36 doc/codewalk/pig.go:36: // stay returns the (successorState, turnIsOver) result of staying. On 2011/03/28 03:25:26, adg wrote: > ditto my comment for roll Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode41 doc/codewalk/pig.go:41: // A policy (or player strategy) is a function from states to actions On 2011/03/28 03:25:26, adg wrote: > How about: "A policy (or player strategy) is a function that chooses an action > for a given pigState." Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode44 doc/codewalk/pig.go:44: // The stayAtKPolicy rolls until k turnPoints are earned and then stays. On 2011/03/28 03:25:26, adg wrote: > s/The // Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode45 doc/codewalk/pig.go:45: func stayAtKPolicy(k int) policy { On 2011/03/28 03:35:01, rsc wrote: > It's a little confusing that stayAtKPolicy is not a policy. > I think it would be fine to call it just stayAtK. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode56 doc/codewalk/pig.go:56: nextPlayer := rand.Intn(2) // Randomly decide who plays first On 2011/03/28 03:25:26, adg wrote: > move this line below the "state :=" line > > maybe it should be currentPlayer ? Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode73 doc/codewalk/pig.go:73: // playPigRecursive simulates a Pig game with a tail-recursive function. On 2011/03/28 03:25:26, adg wrote: > As discussed, this function and its associated commentary should be dropped. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode87 doc/codewalk/pig.go:87: // The total number of wins, losses for policy i are returned in wins[i], losses[i] On 2011/03/28 03:35:01, rsc wrote: > Parallel arrays are almost always a sign that > a you should be using a real data structure > (or that you are a Fortran programmer). > > wins[i] + losses[i] == a constant, right? > So maybe one can be dropped? losses are dropped here, then reconstructed for printing. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode88 doc/codewalk/pig.go:88: func roundRobin(game func(policy, policy) int, policies []policy) ([]int, []int) { On 2011/03/28 03:25:26, adg wrote: > It seems an unnecessary complexity for roundRobin to take a game parameter. Just > hard-code the call to playPig instead. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode92 doc/codewalk/pig.go:92: matchWins := make([]int, 2) // matchWins[0] holds win count for i On 2011/03/28 03:25:26, adg wrote: > s/match/pair/ ? The word "match" is overloaded in English. s/match/series/ instead. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode94 doc/codewalk/pig.go:94: matchWins[game(policies[i], policies[j])]++ On 2011/03/28 03:25:26, adg wrote: > perhaps split this into two lines: > > winner := game(policies[i], policies[j]) > matchWins[winner]++ Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode105 doc/codewalk/pig.go:105: // ratioString joins each element annotated by its fraction of the total. On 2011/03/28 03:25:26, adg wrote: > I know "join" is the common term in functional programming, but it's not really > what's happening here. > > Perhaps something like: > > ratioString takes a list of integer values and returns a string that lists each > value and its percentage of the sum of all values. Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode112 doc/codewalk/pig.go:112: var fmtString string On 2011/03/28 03:35:01, rsc wrote: > This is a somewhat convoluted way to use Sprintf. > It would be simpler to accumulate the actual string: > > s := "" > for i, val := range vals { > if i > 0 { > s += ", " > } > pct := 100*float64(val)/float64(total) > s += fmt.Sprintf("%d/%d (%0.1f%%)", val, total, pct) > } > return s > Done. I went out of my way to make a variadic (args...) call, but it turned out ugly. Weird call dropped, along with the associated codewalk step. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode123 doc/codewalk/pig.go:123: for k := 0; k < *pigGoal; k++ { On 2011/03/28 03:25:26, adg wrote: > for k := range policies { Done. http://codereview.appspot.com/4306045/diff/4001/doc/codewalk/pig.go#newcode128 doc/codewalk/pig.go:128: for k := 0; k < len(policies); k++ { On 2011/03/28 03:25:26, adg wrote: > for k := range policies { Done.
Sign in to reply to this message.
Hello adg, rsc, iant2 (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
some quick comments http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml File doc/codewalk/functions.xml (right): http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml#n... doc/codewalk/functions.xml:18: Pig is a two-player game played with 6-sided dice. Each turn, you may roll or stay. singular "played with a 6-sided die." http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml#n... doc/codewalk/functions.xml:86: Functions can be compared in Go. From the they can only be compared for equality. there is no ordering. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml#n... doc/codewalk/functions.xml:112: href="http://www.google.com/search?q=optimal+play+pig"> optimal policy for you shouldn't need spaces inside the anchor tags. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode13 doc/codewalk/pig.go:13: goal int = 100 // The winning score in a game of Pig "goal" is a confusing name. i'd call this "win" since "int" is the default type for an integer constant, it's idiomatic to leave it off win = 100 http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode19 doc/codewalk/pig.go:19: type state struct { state is an unhelpful name. i'd use score. combined with the suggestion below, the naming works much better http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode20 doc/codewalk/pig.go:20: playerScore, opponentScore, turnPoints int score, score, points. why the inconsistency? maybe just player, opponent, thisTurn int
Sign in to reply to this message.
http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode59 doc/codewalk/pig.go:59: state := state{0, 0, 0} // Initial game state it's fine but maybe unnecessarily subtle to have the var and the type have the same name. also, since you have the zero value, you could write var state state (with better names) if you wanted, but this is ok too. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode98 doc/codewalk/pig.go:98: var total int total := 0 http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode121 doc/codewalk/pig.go:121: fmt.Printf("Wins, losses staying at k=%03d: %s\n", maybe k=% 3d. you don't want leading zeros.
Sign in to reply to this message.
Hello adg, rsc, iant2, r (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
The code is very nice now. http://codereview.appspot.com/4306045/diff/16003/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/16003/doc/codewalk/pig.go#newcode43 doc/codewalk/pig.go:43: // A policy (or player strategy) chooses an action for any given score. Given the parenthetical, why not just call it a strategy? Also the s in the prototype is unnecessary. type strategy func(score) action http://codereview.appspot.com/4306045/diff/16003/doc/codewalk/pig.go#newcode118 doc/codewalk/pig.go:118: wins, gamesPerPolicy := roundRobin(policies) s/gamesPerPolicy/games/ then its games - wins[k] below, which seems nice and clear. (after all it is not winsPerPolicy)
Sign in to reply to this message.
Hello adg, rsc, iant2, r (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thanks. All good suggestions. Sorry for the trigger-happy multiple mailings. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml File doc/codewalk/functions.xml (right): http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml#n... doc/codewalk/functions.xml:18: Pig is a two-player game played with 6-sided dice. Each turn, you may roll or stay. On 2011/04/07 17:45:10, r wrote: > singular > "played with a 6-sided die." Done. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml#n... doc/codewalk/functions.xml:86: Functions can be compared in Go. From the On 2011/04/07 17:45:10, r wrote: > they can only be compared for equality. there is no ordering. Done. Added "for equality". http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/functions.xml#n... doc/codewalk/functions.xml:112: href="http://www.google.com/search?q=optimal+play+pig"> optimal policy for On 2011/04/07 17:45:10, r wrote: > you shouldn't need spaces inside the anchor tags. Done. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode13 doc/codewalk/pig.go:13: goal int = 100 // The winning score in a game of Pig On 2011/04/07 17:45:10, r wrote: > "goal" is a confusing name. i'd call this "win" > > since "int" is the default type for an integer constant, it's idiomatic to leave > it off > win = 100 Done. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode19 doc/codewalk/pig.go:19: type state struct { On 2011/04/07 17:45:10, r wrote: > state is an unhelpful name. > i'd use score. combined with the suggestion below, the naming works much better Done. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode20 doc/codewalk/pig.go:20: playerScore, opponentScore, turnPoints int On 2011/04/07 17:45:10, r wrote: > score, score, points. why the inconsistency? > > maybe just > player, opponent, thisTurn int Done. Also s/successor state/result score throughout. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode59 doc/codewalk/pig.go:59: state := state{0, 0, 0} // Initial game state On 2011/04/07 18:23:51, r wrote: > it's fine but maybe unnecessarily subtle to have the var and the type have the > same name. > also, since you have the zero value, you could write > var state state > (with better names) if you wanted, but this is ok too. Done. The score var is now called s, which is consistent with roll and stay. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode98 doc/codewalk/pig.go:98: var total int On 2011/04/07 18:23:51, r wrote: > total := 0 Done. http://codereview.appspot.com/4306045/diff/15001/doc/codewalk/pig.go#newcode121 doc/codewalk/pig.go:121: fmt.Printf("Wins, losses staying at k=%03d: %s\n", On 2011/04/07 18:23:51, r wrote: > maybe k=% 3d. you don't want leading zeros. Done.
Sign in to reply to this message.
Hello adg, rsc, iant2, r (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thanks. http://codereview.appspot.com/4306045/diff/16003/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/16003/doc/codewalk/pig.go#newcode43 doc/codewalk/pig.go:43: // A policy (or player strategy) chooses an action for any given score. On 2011/04/07 21:43:38, rsc wrote: > Given the parenthetical, why not just call it a strategy? > Also the s in the prototype is unnecessary. > > type strategy func(score) action Policy is more precise, but also more obscure, hence the parenthetical remark. http://codereview.appspot.com/4306045/diff/16003/doc/codewalk/pig.go#newcode118 doc/codewalk/pig.go:118: wins, gamesPerPolicy := roundRobin(policies) On 2011/04/07 21:43:38, rsc wrote: > s/gamesPerPolicy/games/ > then its games - wins[k] below, > which seems nice and clear. > > (after all it is not winsPerPolicy) > Done.
Sign in to reply to this message.
>> Given the parenthetical, why not just call it a strategy? >> Also the s in the prototype is unnecessary. > >> type strategy func(score) action > > Policy is more precise, but also more obscure, hence the parenthetical > remark. Sure but are we trying to teach precise game theory or Go? :-)
Sign in to reply to this message.
i agree with russ that 'strategy' is a better word than policy, but it's not a deal breaker. this is very nice. http://codereview.appspot.com/4306045/diff/18002/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/18002/doc/codewalk/pig.go#newcode106 doc/codewalk/pig.go:106: } you don't need i for _, val := range vals { if s != "" ...
Sign in to reply to this message.
Hello adg, rsc, iant2, r (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Outvoted. "Strategy" it is. http://codereview.appspot.com/4306045/diff/18002/doc/codewalk/pig.go File doc/codewalk/pig.go (right): http://codereview.appspot.com/4306045/diff/18002/doc/codewalk/pig.go#newcode106 doc/codewalk/pig.go:106: } On 2011/04/07 21:56:53, r wrote: > you don't need i > > for _, val := range vals { > if s != "" ... Done.
Sign in to reply to this message.
LGTM anyone else have comments before i submit it?
Sign in to reply to this message.
LGTM A great resource. Thanks, John!
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=56f8b1cc5129 *** A codewalk through a simple program that illustrates several aspects of Go functions: function objects, higher-order functions, variadic functions, tail recursion, etc. The example program simulates the game of Pig, a dice game with simple rules but a nontrivial solution. R=adg, rsc, iant2, r CC=golang-dev http://codereview.appspot.com/4306045 Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.
|
