code review 11407043: go.talks: Best Practices in Go

go.talks: Best Practices in Go

[francesc, 2013-07-17 03:25:10]

Hello adg@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.talks

[adg, 2013-07-17 10:50:09]

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:10: From Wikipedia:
this and the next slide would be better as a single slide

* "Best practices"?

Techniques to write Go code that is

- simple, 
- readable,
- maintainable.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:17: * First some code
s/First s/S/

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:21: * Early return for flatter code
"Avoid nesting by handling errors first"

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:25: Flatter code
"Flatter" means two things, avoid

How about "Less nesting means less cognitive load on the reader"

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:29: * DRY: Don't Repeat Yourself
I really dislike the acronym "DRY" because people use it as a religion. Go
actually makes it hard to "DRY" to the extent some people would like. 

How about "Avoiding repetition"

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:33: Improved readability of the `DumpBinary` function.
this line doesn't say much; delete it, and consider adding
"Deploy one-off utility types for simpler code" ?

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:35: * Moving some complexity into BinWriter
"An opportunity"
 
Show the new Write function first, then show the result (or vice versa)

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:39: * Type switch magic
It's not magic
"Type switch to handle special cases"

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:43: * Function adapters for DRY
s/ for DRY//

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:51: * Best practices to avoid frequent bugs
Not sure this heading is necessary

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:53: * What the bug?
"Spot the bug"

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:61: * Range loops
I think these slides are good candidates for deletion. Too obvious.

The previous shadowing examples could go, too.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:71: * Template method pattern in Go
These examples are really unclear. It's not obvious what the purpose is. Also
I'm not familiar with the term "Template method"

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:83: .code bestpractices/templatemethod.go
/START_F/,/START_MAIN/
I have _never_ seen code like this!

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:103: Send expects a `net.Conn`
What does this mean?

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:117: * Best practices for package organization
"Package organization"

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/mock.go
File 2013/bestpractices/mock.go (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/mock.go#...
2013/bestpractices/mock.go:16: net.Conn
Might as well make this an io.Writer implementation. You're only testing Write
anyway.

Or you need a more interesting example.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.go
File 2013/bestpractices/shadow.go (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.g...
2013/bestpractices/shadow.go:6: func initID() (string, error) { return
"magic-id", nil }
use a real example

maybe *rpc.Client ?

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.g...
2013/bestpractices/shadow.go:23: func betterInit() {
FWIW, you can have multiple init functions, so this can be named init too

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
File 2013/bestpractices/shortercode.go (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:27: } else {
I'd drop the else blocks; they're clearly unnecessary and the code will be gross
without them anyway

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:39: func (g *Gopher) DumpBinary2(w io.Writer)
error {
maybe you should put all these in separate files so they can have the same name

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:56: type BinWriter struct {
call it binWriter, as you're unlikely to make this part of an exported interface

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:58: Err error
s/Err/err/

[francesc, 2013-07-17 22:22:02]

PTAL

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:10: From Wikipedia:
On 2013/07/17 10:50:09, adg wrote:
> this and the next slide would be better as a single slide
> 
> * "Best practices"?
> 
> Techniques to write Go code that is
> 
> - simple, 
> - readable,
> - maintainable.

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:17: * First some code
On 2013/07/17 10:50:09, adg wrote:
> s/First s/S/

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:21: * Early return for flatter code
On 2013/07/17 10:50:09, adg wrote:
> "Avoid nesting by handling errors first"

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:25: Flatter code
On 2013/07/17 10:50:09, adg wrote:
> "Flatter" means two things, avoid
> 
> How about "Less nesting means less cognitive load on the reader"

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:29: * DRY: Don't Repeat Yourself
On 2013/07/17 10:50:09, adg wrote:
> I really dislike the acronym "DRY" because people use it as a religion. Go
> actually makes it hard to "DRY" to the extent some people would like. 
> 
> How about "Avoiding repetition"

I think that DRY is good as long as you understand it's not the top priority.

Between DRY or KISS, go with KISS. I feel like leaving it this way and adding
that as a comment when I'm presenting.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:33: Improved readability of the `DumpBinary` function.
On 2013/07/17 10:50:09, adg wrote:
> this line doesn't say much; delete it, and consider adding
> "Deploy one-off utility types for simpler code" ? 

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:35: * Moving some complexity into BinWriter
On 2013/07/17 10:50:09, adg wrote:
> "An opportunity"
>  
> Show the new Write function first, then show the result (or vice versa)

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:39: * Type switch magic
On 2013/07/17 10:50:09, adg wrote:
> It's not magic
> "Type switch to handle special cases"

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:43: * Function adapters for DRY
On 2013/07/17 10:50:09, adg wrote:
> s/ for DRY//

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:51: * Best practices to avoid frequent bugs
On 2013/07/17 10:50:09, adg wrote:
> Not sure this heading is necessary

Commented out with the rest of the section

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:53: * What the bug?
On 2013/07/17 10:50:09, adg wrote:
> "Spot the bug"

I personally like "what the bug" as it's slightly funny without being offensive.

It reminds me a bit of "go fmt yourself", but with non offensive
interpretations: what the heck?

But if you think that the mood in OSCON is rather serious I'll change it

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:61: * Range loops
On 2013/07/17 10:50:09, adg wrote:
> I think these slides are good candidates for deletion. Too obvious.
> 
> The previous shadowing examples could go, too.

Commented out for the moment.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:71: * Template method pattern in Go
On 2013/07/17 10:50:09, adg wrote:
> These examples are really unclear. It's not obvious what the purpose is. Also
> I'm not familiar with the term "Template method"

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:83: .code bestpractices/templatemethod.go
/START_F/,/START_MAIN/
On 2013/07/17 10:50:09, adg wrote:
> I have _never_ seen code like this!

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:103: Send expects a `net.Conn`
On 2013/07/17 10:50:09, adg wrote:
> What does this mean?

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:117: * Best practices for package organization
On 2013/07/17 10:50:09, adg wrote:
> "Package organization"

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/mock.go
File 2013/bestpractices/mock.go (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/mock.go#...
2013/bestpractices/mock.go:16: net.Conn
On 2013/07/17 10:50:09, adg wrote:
> Might as well make this an io.Writer implementation. You're only testing Write
> anyway.
> 
> Or you need a more interesting example.

The point is that net.Conn has more methods, but I only have to redefine those
that the tested method uses.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.go
File 2013/bestpractices/shadow.go (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.g...
2013/bestpractices/shadow.go:6: func initID() (string, error) { return
"magic-id", nil }
On 2013/07/17 10:50:09, adg wrote:
> use a real example
> 
> maybe *rpc.Client ?

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.g...
2013/bestpractices/shadow.go:23: func betterInit() {
On 2013/07/17 10:50:09, adg wrote:
> FWIW, you can have multiple init functions, so this can be named init too

Yeah, but then the code wouldn't fail

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
File 2013/bestpractices/shortercode.go (right):

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:27: } else {
On 2013/07/17 10:50:09, adg wrote:
> I'd drop the else blocks; they're clearly unnecessary and the code will be
gross
> without them anyway

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:39: func (g *Gopher) DumpBinary2(w io.Writer)
error {
On 2013/07/17 10:50:09, adg wrote:
> maybe you should put all these in separate files so they can have the same
name

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:56: type BinWriter struct {
On 2013/07/17 10:50:09, adg wrote:
> call it binWriter, as you're unlikely to make this part of an exported
interface

Done.

https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode.go:58: Err error
On 2013/07/17 10:50:09, adg wrote:
> s/Err/err/

Done.

[campoy, 2013-07-23 06:29:32]

On 2013/07/17 22:22:02, gocampoy wrote:
> PTAL
> 
> https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide
> File 2013/bestpractices.slide (right):
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:10: From Wikipedia:
> On 2013/07/17 10:50:09, adg wrote:
> > this and the next slide would be better as a single slide
> > 
> > * "Best practices"?
> > 
> > Techniques to write Go code that is
> > 
> > - simple, 
> > - readable,
> > - maintainable.
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:17: * First some code
> On 2013/07/17 10:50:09, adg wrote:
> > s/First s/S/
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:21: * Early return for flatter code
> On 2013/07/17 10:50:09, adg wrote:
> > "Avoid nesting by handling errors first"
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:25: Flatter code
> On 2013/07/17 10:50:09, adg wrote:
> > "Flatter" means two things, avoid
> > 
> > How about "Less nesting means less cognitive load on the reader"
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:29: * DRY: Don't Repeat Yourself
> On 2013/07/17 10:50:09, adg wrote:
> > I really dislike the acronym "DRY" because people use it as a religion. Go
> > actually makes it hard to "DRY" to the extent some people would like. 
> > 
> > How about "Avoiding repetition"
> 
> I think that DRY is good as long as you understand it's not the top priority.
> 
> Between DRY or KISS, go with KISS. I feel like leaving it this way and adding
> that as a comment when I'm presenting.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:33: Improved readability of the `DumpBinary`
function.
> On 2013/07/17 10:50:09, adg wrote:
> > this line doesn't say much; delete it, and consider adding
> > "Deploy one-off utility types for simpler code" ? 
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:35: * Moving some complexity into BinWriter
> On 2013/07/17 10:50:09, adg wrote:
> > "An opportunity"
> >  
> > Show the new Write function first, then show the result (or vice versa)
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:39: * Type switch magic
> On 2013/07/17 10:50:09, adg wrote:
> > It's not magic
> > "Type switch to handle special cases"
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:43: * Function adapters for DRY
> On 2013/07/17 10:50:09, adg wrote:
> > s/ for DRY//
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:51: * Best practices to avoid frequent bugs
> On 2013/07/17 10:50:09, adg wrote:
> > Not sure this heading is necessary
> 
> Commented out with the rest of the section
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:53: * What the bug?
> On 2013/07/17 10:50:09, adg wrote:
> > "Spot the bug"
> 
> I personally like "what the bug" as it's slightly funny without being
offensive.
> 
> It reminds me a bit of "go fmt yourself", but with non offensive
> interpretations: what the heck?
> 
> But if you think that the mood in OSCON is rather serious I'll change it
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:61: * Range loops
> On 2013/07/17 10:50:09, adg wrote:
> > I think these slides are good candidates for deletion. Too obvious.
> > 
> > The previous shadowing examples could go, too.
> 
> Commented out for the moment.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:71: * Template method pattern in Go
> On 2013/07/17 10:50:09, adg wrote:
> > These examples are really unclear. It's not obvious what the purpose is.
Also
> > I'm not familiar with the term "Template method"
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:83: .code bestpractices/templatemethod.go
> /START_F/,/START_MAIN/
> On 2013/07/17 10:50:09, adg wrote:
> > I have _never_ seen code like this!
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:103: Send expects a `net.Conn`
> On 2013/07/17 10:50:09, adg wrote:
> > What does this mean?
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices.slide#ne...
> 2013/bestpractices.slide:117: * Best practices for package organization
> On 2013/07/17 10:50:09, adg wrote:
> > "Package organization"
> 
> Done.
> 
> https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/mock.go
> File 2013/bestpractices/mock.go (right):
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/mock.go#...
> 2013/bestpractices/mock.go:16: net.Conn
> On 2013/07/17 10:50:09, adg wrote:
> > Might as well make this an io.Writer implementation. You're only testing
Write
> > anyway.
> > 
> > Or you need a more interesting example.
> 
> The point is that net.Conn has more methods, but I only have to redefine those
> that the tested method uses.
> 
> https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.go
> File 2013/bestpractices/shadow.go (right):
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.g...
> 2013/bestpractices/shadow.go:6: func initID() (string, error) { return
> "magic-id", nil }
> On 2013/07/17 10:50:09, adg wrote:
> > use a real example
> > 
> > maybe *rpc.Client ?
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shadow.g...
> 2013/bestpractices/shadow.go:23: func betterInit() {
> On 2013/07/17 10:50:09, adg wrote:
> > FWIW, you can have multiple init functions, so this can be named init too
> 
> Yeah, but then the code wouldn't fail
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
> File 2013/bestpractices/shortercode.go (right):
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
> 2013/bestpractices/shortercode.go:27: } else {
> On 2013/07/17 10:50:09, adg wrote:
> > I'd drop the else blocks; they're clearly unnecessary and the code will be
> gross
> > without them anyway
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
> 2013/bestpractices/shortercode.go:39: func (g *Gopher) DumpBinary2(w
io.Writer)
> error {
> On 2013/07/17 10:50:09, adg wrote:
> > maybe you should put all these in separate files so they can have the same
> name
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
> 2013/bestpractices/shortercode.go:56: type BinWriter struct {
> On 2013/07/17 10:50:09, adg wrote:
> > call it binWriter, as you're unlikely to make this part of an exported
> interface
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/6001/2013/bestpractices/shorterc...
> 2013/bestpractices/shortercode.go:58: Err error
> On 2013/07/17 10:50:09, adg wrote:
> > s/Err/err/
> 
> Done.

PTAL

[adg, 2013-07-23 07:45:03]

https://codereview.appspot.com/11407043/diff/13001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/11407043/diff/13001/2013/bestpractices.slide#n...
2013/bestpractices.slide:10: From Wikipedia:
drop the wikipedia quote. it's a terrible source for anything

https://codereview.appspot.com/11407043/diff/13001/2013/bestpractices.slide#n...
2013/bestpractices.slide:31: Less nesting means less cognitive load on the
reader
period

https://codereview.appspot.com/11407043/diff/13001/2013/bestpractices.slide#n...
2013/bestpractices.slide:47: * Type switch with short variable declaration
I'd consider dropping this slide and putting the short var decl in the previous
one. It's the natural way anyway, not a 'best practice'.

https://codereview.appspot.com/11407043/diff/13001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode1.go (right):

https://codereview.appspot.com/11407043/diff/13001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:31: 
delete blank line

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:38: Deploy one-off utility types for simpler code
move this above the first code sample

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:150: Import statements, cohesive groups separated by
blank lines.
s/cohesive/related/

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:160: Constants, functions, and types used across the
rest of the code.
Not sure how this is distinct from "other" (below), also you forgot variables.
I think you should instead draw a distinction between internal and exported
declarations.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:172: Reasons to separate code in different files.
"Why split a package into multiple files?"

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:180: `net/http/cookie.go` and `net/http/cookie_test.go`
are both part of the `http`
Mention that tests are part of the package name space, but are only compiled at
test time.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:209: .image bestpractices/cmd.png
missing image

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:211: * Keeping independent packages independent
s/Keeping/Keep/

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:215: They can independent if `b` defines an interface
with the needed methods of `a.T`.
Pick a concrete example? Or at least more informative names. I'm lost looking at
this code

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:246: - In package `encoding/json` we find the type
`Encoder` instead of `JSONEncoder`.
s/instead of/, not/

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:248: - It is referred as `json.Encoder`, shorter than
`json.JSONEncoder`, but also clear.
It is used as `json.Encoder`.

(the rest is obvious)

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:254: .code bestpractices/shortercode1.go
/GOPHER/,/DUMP/
you use pointers to interfaces a lot throughout this code

look for *io in your code samples

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:282: * goroutines attached to values
use goroutines to manage state

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:292: * avoid goroutines leaks with buffered chans
s/goroutines/goroutine/

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:294: .play bestpractices/bufchan.go /START/,
use the classic example here; a chan of errors. it's the most widely applicable

[campoy, 2013-07-23 23:37:16]

PTAL

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:38: Deploy one-off utility types for simpler code
On 2013/07/23 07:45:04, adg wrote:
> move this above the first code sample

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:150: Import statements, cohesive groups separated by
blank lines.
On 2013/07/23 07:45:04, adg wrote:
> s/cohesive/related/

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:160: Constants, functions, and types used across the
rest of the code.
On 2013/07/23 07:45:04, adg wrote:
> Not sure how this is distinct from "other" (below), also you forgot variables.
> I think you should instead draw a distinction between internal and exported
> declarations.

Constants and helper functions are normally at the beginning of the package (or
at least that's what I normally see). That's what I wanted to express.

I reworded it.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:172: Reasons to separate code in different files.
On 2013/07/23 07:45:04, adg wrote:
> "Why split a package into multiple files?"

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:180: `net/http/cookie.go` and `net/http/cookie_test.go`
are both part of the `http`
On 2013/07/23 07:45:04, adg wrote:
> Mention that tests are part of the package name space, but are only compiled
at
> test time.

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:209: .image bestpractices/cmd.png
On 2013/07/23 07:45:04, adg wrote:
> missing image

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:211: * Keeping independent packages independent
On 2013/07/23 07:45:04, adg wrote:
> s/Keeping/Keep/

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:215: They can independent if `b` defines an interface
with the needed methods of `a.T`.
On 2013/07/23 07:45:04, adg wrote:
> Pick a concrete example? Or at least more informative names. I'm lost looking
at
> this code

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:246: - In package `encoding/json` we find the type
`Encoder` instead of `JSONEncoder`.
On 2013/07/23 07:45:04, adg wrote:
> s/instead of/, not/

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:248: - It is referred as `json.Encoder`, shorter than
`json.JSONEncoder`, but also clear.
On 2013/07/23 07:45:04, adg wrote:
> It is used as `json.Encoder`.
> 
> (the rest is obvious)

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:254: .code bestpractices/shortercode1.go
/GOPHER/,/DUMP/
On 2013/07/23 07:45:04, adg wrote:
> you use pointers to interfaces a lot throughout this code
> 
> look for *io in your code samples

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:282: * goroutines attached to values
On 2013/07/23 07:45:04, adg wrote:
> use goroutines to manage state

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:292: * avoid goroutines leaks with buffered chans
On 2013/07/23 07:45:04, adg wrote:
> s/goroutines/goroutine/

Done.

https://codereview.appspot.com/11407043/diff/22001/2013/bestpractices.slide#n...
2013/bestpractices.slide:294: .play bestpractices/bufchan.go /START/,
On 2013/07/23 07:45:04, adg wrote:
> use the classic example here; a chan of errors. it's the most widely
applicable

Done.

[adg, 2013-07-24 01:46:19]

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:160: Constants, variables, and helper functions and
types.
I would say that helper functions and types go at the bottom. The most
significant types and funcs should go at the top.

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:269: * Avoid concurrency on your API
s/on/in/

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:273: What if we want to use it concurrently?
this is a non-sequitur, considering you're using it concurrently already

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:275: * Avoid concurrency on your API
ditto

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:285: Use a chan or a struct with a chan as a handle to
a goroutine
s/as a handle to/to communicate with/

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:289: * goroutines attached to values (cont.)
"Use goroutines to manage state (continued)"

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:293: * avoid goroutine leaks with buffered chans
bad capitalization

[campoy, 2013-07-24 18:30:05]

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:160: Constants, variables, and helper functions and
types.
On 2013/07/24 01:46:19, adg wrote:
> I would say that helper functions and types go at the bottom. The most
> significant types and funcs should go at the top.

Done.

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:269: * Avoid concurrency on your API
On 2013/07/24 01:46:19, adg wrote:
> s/on/in/

Done.

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:273: What if we want to use it concurrently?
On 2013/07/24 01:46:19, adg wrote:
> this is a non-sequitur, considering you're using it concurrently already

Done.

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:275: * Avoid concurrency on your API
On 2013/07/24 01:46:19, adg wrote:
> ditto

Done.

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:285: Use a chan or a struct with a chan as a handle to
a goroutine
On 2013/07/24 01:46:19, adg wrote:
> s/as a handle to/to communicate with/

Done.

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:289: * goroutines attached to values (cont.)
On 2013/07/24 01:46:19, adg wrote:
> "Use goroutines to manage state (continued)"

Done.

https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
2013/bestpractices.slide:293: * avoid goroutine leaks with buffered chans
On 2013/07/24 01:46:19, adg wrote:
> bad capitalization

Done.

[campoy, 2013-07-24 23:44:03]

On 2013/07/24 18:30:05, campoy wrote:
> https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide
> File 2013/bestpractices.slide (right):
> 
>
https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
> 2013/bestpractices.slide:160: Constants, variables, and helper functions and
> types.
> On 2013/07/24 01:46:19, adg wrote:
> > I would say that helper functions and types go at the bottom. The most
> > significant types and funcs should go at the top.
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
> 2013/bestpractices.slide:269: * Avoid concurrency on your API
> On 2013/07/24 01:46:19, adg wrote:
> > s/on/in/
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
> 2013/bestpractices.slide:273: What if we want to use it concurrently?
> On 2013/07/24 01:46:19, adg wrote:
> > this is a non-sequitur, considering you're using it concurrently already
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
> 2013/bestpractices.slide:275: * Avoid concurrency on your API
> On 2013/07/24 01:46:19, adg wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
> 2013/bestpractices.slide:285: Use a chan or a struct with a chan as a handle
to
> a goroutine
> On 2013/07/24 01:46:19, adg wrote:
> > s/as a handle to/to communicate with/
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
> 2013/bestpractices.slide:289: * goroutines attached to values (cont.)
> On 2013/07/24 01:46:19, adg wrote:
> > "Use goroutines to manage state (continued)"
> 
> Done.
> 
>
https://codereview.appspot.com/11407043/diff/33001/2013/bestpractices.slide#n...
> 2013/bestpractices.slide:293: * avoid goroutine leaks with buffered chans
> On 2013/07/24 01:46:19, adg wrote:
> > bad capitalization
> 
> Done.

PTAL

I just uploaded the pretty much what I presented. I  ended up dropping some
slides to make the talk fit in 30 minutes.

[adg, 2013-07-24 23:47:06]

LGTM

[francesc, 2013-07-24 23:49:44]

*** Submitted as
https://code.google.com/p/go/source/detail?r=dd84e6e4aecd&repo=talks ***

go.talks: Best Practices in Go

R=adg, campoy
CC=golang-dev
https://codereview.appspot.com/11407043

[campoy, 2013-07-26 21:19:11]

Hi Christoph,

My code wasn't intended to be safe in the case of multiple concurrent calls
to stop, it's something I knew.

And yes, I agree that your solution fixes it :-) Nice job!

 Francesc Campoy Flores | Go Developer Relations | campoy@google.com |
 415-990-4126


On Wed, Jul 24, 2013 at 6:24 PM, Christoph Hack <christoph@tux21b.org>wrote:

> Sorry for yet another mail, but I think I have found a cleaner solution
> based on sync.Once. I am happy now with this approach:
> http://play.golang.org/p/KAn1Y8uczj
>
> Am Donnerstag, 25. Juli 2013 02:54:57 UTC+2 schrieb Christoph Hack:
>
>> The "Use goroutines to manage state" example code looks a bit racy to me.
>> The Stop method must not be called concurrently or otherwise clients might
>> either observe a stopped state before the server was shut down properly or
>> they may block indefinitely if someone else stopped the server before. I am
>> guessing that's not the intended behavior, since the whole purpose of the
>> dedicated server goroutine is to allow concurrent access, isn't it?
>>
>>
http://play.golang.org/p/**l9s7SZZBJb<http://play.golang.org/p/l9s7SZZBJb>should
fix the problem, but I am not sure if I would consider it best
>> practice. Does anyone know a simpler solution to this common problem?
>>
>