<p>Hi, I have about 8-9 years of exp in web dev, company is moving to use go from php. I decided to try to learn go by writing out the classic card game war.<br/>
After playing around with it a bit, I decided to see what the community would say and comment about. </p>
<p>The URL for the repo is: <a href="https://github.com/smergler/war_go" rel="nofollow">https://github.com/smergler/war_go</a></p>
<p>If this is the wrong subreddit, please let me know. </p>
<p>Thanks!</p>
<hr/>**评论：**<br/><br/>DenzelM: <pre><p>Welcome Sloan! This is absolutely the right subreddit, as evidenced by all the helpful comments so far. You're already well on your way, so I'm going to try to provide you with some guidance on how to jump to the next level.</p>
<p>To start off, you did a great job separating responsibilities, putting a structure in place, and writing understandable tests. It's very easy to follow the flow of logic in your code and your tests.</p>
<p>I'm going to attack this in two ways. First, I'm simply going to treat the code as-is and comment on syntactical and semantic improvements; ways to improve the idiomatic character of the code. Then, I'm going to comment on places where I believe structural improvements can be made.</p>
<p>Before I do that, there are some minor code formatting issues. Luckily, formatting can be taken care of in an automated fashion with <a href="https://blog.golang.org/go-fmt-your-code" rel="nofollow">Go's gofmt tool</a>.</p>
<p>Ok, let's talk about the syntax/semantics:</p>
<ul>
<li>Slice expressions</li>
<li>Appending one slice to another slice</li>
<li>For-Range loops</li>
<li>Print() with String()</li>
<li>Implicit/explicit initialization of empty types</li>
<li>Pointer vs. value types for structures</li>
</ul>
<p><strong>Slice expressions</strong></p>
<p>When re-slicing a slice in the case of expressions like <code>s[1:len(s)]</code>, you can simplify to <code>s[1:]</code> which implies slicing from one to the end. <a href="https://golang.org/ref/spec#Slice_expressions" rel="nofollow">Slice expressions</a> is a great section, in the Go specification, on all the ways you can slice and dice.</p>
<p><strong>Appending one slice to another slice</strong></p>
<p>Go provides some <a href="https://golang.org/ref/spec#Appending_and_copying_slices" rel="nofollow">simple syntax</a> for appending one slice to another too; making your life a lot easier. (Pro tip: you can actually use this <a href="https://golang.org/ref/spec#Passing_arguments_to_..._parameters" rel="nofollow">destructuring-like syntax</a> for any variadic function.)</p>
<p>Transforming:</p>
<pre><code>for i := 0; i < len(cardsInPlay); i++ {
p_winner.Deck = append(p_winner.Deck, cardsInPlay[i])
}
</code></pre>
<p>Into:</p>
<pre><code>p_winner.Deck = append(p_winner.Deck, cardsInPlay...)
</code></pre>
<p><strong>For-Range loops</strong></p>
<p>Go provides a useful <a href="https://golang.org/ref/spec#RangeClause" rel="nofollow">range clause</a> for <a href="https://golang.org/doc/effective_go.html#for" rel="nofollow">iterating</a> over the values of an array, slice, string, map, or even a channel. It saves the tedium of writing a loop and using an index to iterate over data.</p>
<p>Transforming:</p>
<pre><code>for suit_num := 0; suit_num < len(suits); suit_num++ {
// suit = suits[suit_num]
}
</code></pre>
<p>Into:</p>
<pre><code>for _, suit := range suits {
// ...
}
</code></pre>
<p><strong>Print() with String()</strong></p>
<p>The <a href="https://golang.org/pkg/fmt/#Stringer" rel="nofollow">Stringer</a> interface, which is implemented by any type that has a <code>String() string</code> method, is used to print values passed as an operand to any formatting method like <code>Printf()</code> or even unformatted methods like <code>Print()</code>.</p>
<p>Since you implemented the <em>Stringer</em> interface for you <em>Card</em> type, that makes this redundant:</p>
<pre><code>fmt.Printf("%s's card: %s\n", h.P1.Name, p1_card.String())
</code></pre>
<p>Instead:</p>
<pre><code>fmt.Printf("%s's card: %s\n", h.P1.Name, p1_card)
</code></pre>
<p>Works just as well.</p>
<p><strong>Implicit/explicit initialization of empty types</strong></p>
<p>Whenever you declare or construct a new variable it will be initialized with its <a href="https://golang.org/ref/spec#The_zero_value" rel="nofollow">zero value</a>, if you choose not to explicitly initialize it. Every basic type has a zero value, and the zero value of a composite type is made up of the zero values of its constituent parts.</p>
<p>For example, the zero value of any string is "". Therefore, when declaring <code>var s string</code>, the variable <em>s</em> is implicitly initialized to the zero value of "" without having to explicitly initialize it. We can use this property with structures, a composite type, as well. So that:</p>
<pre><code>return Card{
Rank: "",
Suit: "",
}
</code></pre>
<p>Becomes:</p>
<pre><code>return Card{}
</code></pre>
<p>And the constituent parts, <em>Rank</em> and <em>Suit</em>, will be initialized to their respective zero values (the empty string in this case). You can use this fact to your advantage many times to simplify your code.</p>
<p><strong>Pointer vs. value types for structures</strong></p>
<p>Generally, when you're working with composite types, you want to pass them around as pointers instead of values. As long as these composite types are not opaque references, e.g., slices, maps, etc. It's far more efficient to pass around a pointer to a structure instead of copying the value of the structure for each procedure call or return.</p>
<p>When you pass a pointer, the runtime only has to copy over 4 bytes (for a 32-bit system) or 8 bytes (for a 64-bit system). When you pass the whole structure by value, it has to copy over N bytes, where N is however many bytes the entire structure takes up in memory.</p>
<p>Now, let's talk about some of the structural/architectural improvements that can be made. You're already 80-90% of the way there, and we're going to explore taking it even further:</p>
<ul>
<li>Go type crazy</li>
<li>Representing Rank and Suit</li>
<li>Yank Deck from Player</li>
<li>Represent a Pile of Cards?</li>
<li>Type constructors</li>
</ul>
<p><strong>Go type crazy</strong></p>
<p>Go's types are so lightweight. They make it easy to represent any concept you want. I find it really helpful to represent even the smallest concepts in the domain of what I'm working on with a type.</p>
<p>Go's types are very lightweight. It's easy to represent any concept you want. I find it really helpful in representing even the smallest domain concepts in projects I'm working on. If <code>[]Card</code> represents a deck of cards, then why not call it as such with a <code>type Deck []Card</code> declaration. Easy enough. And that illuminates a problem doesn't it... because not just any amount of cards is a deck? In fact, a Deck represents a type concept with very specific type invariants. More on this idea later.</p>
<p>Let's talk about a better representation for the rank and suit of a card.</p>
<p><strong>Representing Rank and Suit</strong></p>
<p>Remember, our types should represent our domain concepts. So let's name them as such:</p>
<pre><code>type Suit string
type Rank string
</code></pre>
<p>But we can make things even better. Let's represent <em>Rank</em> as an integer, and then convert that to a name later. You've got the right idea, by constructing a map from a cards name to its rank. We're going to need a way to translate between the name and value of ranks if we want to print it out later. However, let's invert the representation. If you think about it, that's what a rank really is, an integer value. We'll make it <code>type Rank int</code>.</p>
<p>Now, we're going to have to create the actual ranks. We can do that with Go's convenient little <a href="https://golang.org/ref/spec#Iota" rel="nofollow">iota identifier</a> to create successive, increasing integers. We're going to create a series of <a href="https://golang.org/doc/effective_go.html#constants" rel="nofollow">constants</a> that work sort of like a typed enumeration.</p>
<pre><code>const (
Ace Rank = iota
One
Two
// ...
King
)
</code></pre>
<p>That way we can do things like:</p>
<pre><code>r1 := Rank{Jack}
r2 := Rank{King}
r1 > r2
</code></pre>
<p>We can even implement the <em>Stringer</em> interface on <em>Rank</em> itself and use that when printing out the rank as a string. But how are we going to translate from an integer value to a string. Well, we can just flip your idea from before, create a package-private map to translate from <em>Rank</em> to <em>string</em>, and then use that for the string method. Something like:</p>
<pre><code>var rankNames = map[Rank]string{
Ace: "A",
King: "K",
// ...
One: "1",
}
func (r Rank) String() string { return rankNames[r] }
</code></pre>
<p>I'll leave the update of <em>Card</em> and the <em>IsBetterThan</em> method to you. It should be a lot easier, more straightforward, and simpler now.</p>
<p><strong>Yank Deck from Player</strong></p>
<p>Let's continue using our type crazy powers. A <em>Player</em> has a deck, but a player isn't a deck itself. Therefore, <em>ShuffleDeck()</em>, <em>IsDeckEmpty()</em>, and <em>DeckSize()</em> should not be defined on a <em>Player</em>. We can extract a deck out into its own type to represent that domain-level concept.</p>
<pre><code>type Deck []Card
</code></pre>
<p>Then we can define methods on that domain-level concept:</p>
<pre><code>func (d *Deck) ShuffleDeck()
func (d *Deck) IsDeckEmpty() bool
func (d *Deck) DeckSize() int
</code></pre>
<p>And use that concept explicitly throughout the program. Which leads us to an interesting question though. There's one more concept that may be especially helpful to represent for a card game like War.</p>
<p><strong>Represent a Pile of Cards?</strong></p>
<p>Did you notice that when you're playing the game of War, that you're representing the middle pile of cards as <code>[]Card</code>. That makes enough sense. But we could also move it to a first level concept! Making:</p>
<pre><code>type Pile []Card
</code></pre>
<p>And then implementing the appropriate methods on a pile of cards. Methods to take away cards, add cards, or even add whole other pile. This makes sense. At the end of the day, the pile of cards in play is quite literally a pile. So then our deck becomes a specific type of pile:</p>
<pre><code>type Deck Pile
</code></pre>
<p>Why would we do that? Well it depends on how detailed you want to be with your domain concepts. But generally, a <em>Deck</em> of cards has a specific invariant that can't be violated. Namely, there can never be >52 cards, and no cards can be repeated. This is an invariant that a <em>Pile</em> of cards does not have. It maps pretty nicely to our idea of how things work in the real-world.</p>
<p><strong>Type constructors</strong></p>
<p>Finally, the logic that you have in <em>Game.SetUpDeck()</em> to create a deck is actually the responsibility of a <a href="https://golang.org/doc/effective_go.html#composite_literals" rel="nofollow">type constructor</a>. A type constructor for <em>Deck</em> will construct it so as to not invalidate any invariants and maintain them throughout the lifecycle of the object.</p>
<p>Idiomatic type constructors in go start with "New" and usually describe the type to be constructed. In the case of your <em>Deck</em> it'd look something like:</p>
<pre><code>func NewDeck() *Deck
</code></pre>
<p><strong>Code Review: Complete</strong></p>
<p>That's everything I have for my code review, Sloan. Hopefully it's been helpful. Feel free to post here again with an update, for feedback on anything else, or just to ask a question. We love to help around here!</p></pre>piratelax40: <pre><p>just wanted to say awesome review - I picked up some new tips and ideas as well</p></pre>xiegeo: <pre><p>For someone from php, the biggest thing to learn is the type system. It takes a while to get, but once you do, you won't want to go back to an untyped language for building any complex system.</p></pre>miko5054: <pre><p>IMO adding interfaces and packages will make the code more organize </p></pre>golang_throwaway: <pre><p>thanks</p></pre>dinkumator: <pre><p>I'd use some types for the suit and ranks. You can have string methods to pretty print them and just integer enums</p></pre>golang_throwaway: <pre><p>thanks, I'll check that out </p></pre>
