code review 12636043: go.talks: bestpractices: adding compelling example for ...

go.talks: bestpractices: adding compelling example for error handling.

[francesc, 2013-08-08 00:22:25]

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

[r, 2013-08-08 00:41:39]

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:55: * Atomic writing
the title is wrong; the writes are not atomic.

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode6.go:29: // Write write the given value into the
writer using little endian.
// Write writes the value using little-endian byte order.

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode6.go:45: // Close actually writes all the previous
values into the writer if no
s/actually //

maybe

// Close writes any pending values into the writer if no error has occurred.
// If an error has occurred, earlier or with a write by Close, the error is
returned.

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode6.go:70: Age:      3383,
is this accurate? :)

[kortschak, 2013-08-08 00:52:15]

On 2013/08/08 00:41:39, r wrote:
 
>
https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
> 2013/bestpractices/shortercode6.go:70: Age:      3383,
> is this accurate? :)

Off by one - years are 1-based.

[francesc, 2013-08-08 04:21:51]

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices.slide
File 2013/bestpractices.slide (right):

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices.slide#ne...
2013/bestpractices.slide:55: * Atomic writing
On 2013/08/08 00:41:39, r wrote:
> the title is wrong; the writes are not atomic.

Better?

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode6.go:29: // Write write the given value into the
writer using little endian.
On 2013/08/08 00:41:39, r wrote:
> // Write writes the value using little-endian byte order.

Done.

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode6.go:45: // Close actually writes all the previous
values into the writer if no
On 2013/08/08 00:41:39, r wrote:
> s/actually //
> 
> maybe
> 
> // Close writes any pending values into the writer if no error has occurred.
> // If an error has occurred, earlier or with a write by Close, the error is
> returned.
> 

Done.

https://codereview.appspot.com/12636043/diff/9001/2013/bestpractices/shorterc...
2013/bestpractices/shortercode6.go:70: Age:      3383,
On 2013/08/08 00:41:39, r wrote:
> is this accurate? :)

Now it is :-)

[r, 2013-08-08 04:39:17]

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:48: func (w *binWriter) Close() error {
this is Flush, not Close. that's why i was confused

[ioe, 2013-08-08 14:46:31]

Please note that the bug(?) noticed is already introduced between
shortercode2.go and shortercode3.go and I wondered about it while watching the
presentation already.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:36: w.Write(int32(len(v)))
Why you do endian conversion for v below but not for the int32 here?

[adg, 2013-08-12 01:21:36]

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:1: // +build ignore,OMIT
+build OMIT will suffice

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:14: // GOPHER OMIT
can't you just use /type Gopher/ as the rule and drop these ugly comments?

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:22: type binWriter struct {
ditto

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:29: // Write writes the value using
little-endian byte order.
ditto, /Write writes/

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:36: w.Write(int32(len(v)))
On 2013/08/08 14:46:31, ioe wrote:
> Why you do endian conversion for v below but not for the int32 here? 

This converts v to an int32 and calls Write again. The data is only actually
written to the buffer by binary.Write. I agree it's unclear.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:52: _, w.err = w.w.Write(w.buf.Bytes())
w.buf.WriteTo(w.w)

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:57: func (g *Gopher) DumpBinary(w io.Writer)
error {
make this an implementation of io.WriterTo

[francesc, 2013-08-12 23:50:25]

Implementing WriterTo made the case for code simplification even better IMHO.

Will clean out the comments for code extraction on the rest of the talk in a
separate CL.

PTAL

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:1: // +build ignore,OMIT
On 2013/08/12 01:21:36, adg wrote:
> +build OMIT will suffice

This was added by cl 12003043 that you LGTMed

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:14: // GOPHER OMIT
On 2013/08/12 01:21:36, adg wrote:
> can't you just use /type Gopher/ as the rule and drop these ugly comments?

Done.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:22: type binWriter struct {
On 2013/08/12 01:21:36, adg wrote:
> ditto

Done.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:29: // Write writes the value using
little-endian byte order.
On 2013/08/12 01:21:36, adg wrote:
> ditto, /Write writes/

Done.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:36: w.Write(int32(len(v)))
On 2013/08/12 01:21:36, adg wrote:
> On 2013/08/08 14:46:31, ioe wrote:
> > Why you do endian conversion for v below but not for the int32 here? 
> 
> This converts v to an int32 and calls Write again. The data is only actually
> written to the buffer by binary.Write. I agree it's unclear.

Done.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:48: func (w *binWriter) Close() error {
On 2013/08/08 04:39:18, r wrote:
> this is Flush, not Close. that's why i was confused

Done.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:52: _, w.err = w.w.Write(w.buf.Bytes())
On 2013/08/12 01:21:36, adg wrote:
> w.buf.WriteTo(w.w)

Done.

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:57: func (g *Gopher) DumpBinary(w io.Writer)
error {
On 2013/08/12 01:21:36, adg wrote:
> make this an implementation of io.WriterTo

Done.

[adg, 2013-08-13 03:30:57]

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/15001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:1: // +build ignore,OMIT
On 2013/08/12 23:50:25, gocampoy wrote:
> On 2013/08/12 01:21:36, adg wrote:
> > +build OMIT will suffice
> 
> This was added by cl 12003043 that you LGTMed

Guess i missed it, or that CL happened before we started using +build OMIT
everywhere.

(There's nothing special about "ignore")

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

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:14: AgeYears int64
why int64?

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:17: func (g *Gopher) WriteTo(w io.Writer)
(size int64, err error) {
maybe you should add a comment to the files where the code is deliberately bad
style (not present in the slides, but here for anyone looking at the file)
I keep wanting to tell you to fix this code, even though I know it's
intentionally ugly.

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode2.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode2.go:23: var n int
you don't need this; you can use :=

http://play.golang.org/p/6IbmSWME0b

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode4.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode4.go:14: AgeYears int32
now int32? just make this an int

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode5.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode5.go:23: // Write writes a value into its writer
using little endian.
writes a value to the provided writer in little endian form.

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:48: func (g *Gopher) WriteTo(w io.Writer)
(int64, error) {
now the doc comment is missing?

i suggest you diff each subsequent pair of these files to make sure they are as
identical as possible

[francesc, 2013-08-15 18:50:41]

PTAL

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

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:14: AgeYears int64
On 2013/08/13 03:30:58, adg wrote:
> why int64?

Done.

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:17: func (g *Gopher) WriteTo(w io.Writer)
(size int64, err error) {
On 2013/08/13 03:30:58, adg wrote:
> maybe you should add a comment to the files where the code is deliberately bad
> style (not present in the slides, but here for anyone looking at the file)
> I keep wanting to tell you to fix this code, even though I know it's
> intentionally ugly.

Done.

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode2.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode2.go:23: var n int
On 2013/08/13 03:30:58, adg wrote:
> you don't need this; you can use :=
> 
> http://play.golang.org/p/6IbmSWME0b

Done.

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode4.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode4.go:14: AgeYears int32
On 2013/08/13 03:30:58, adg wrote:
> now int32? just make this an int
Encoding int into binary requires a type conversion to a fixed size type.

Modified to have int on the struct and encode it as int64.

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode5.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode5.go:23: // Write writes a value into its writer
using little endian.
On 2013/08/13 03:30:58, adg wrote:
> writes a value to the provided writer in little endian form.

Done.

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
File 2013/bestpractices/shortercode6.go (right):

https://codereview.appspot.com/12636043/diff/22001/2013/bestpractices/shorter...
2013/bestpractices/shortercode6.go:48: func (g *Gopher) WriteTo(w io.Writer)
(int64, error) {
On 2013/08/13 03:30:58, adg wrote:
> now the doc comment is missing?
> 
> i suggest you diff each subsequent pair of these files to make sure they are
as
> identical as possible

Done.

[adg, 2013-08-16 03:17:52]

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

https://codereview.appspot.com/12636043/diff/33001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:38: var g io.WriterTo = &Gopher{
this is a weird variable declaration. i don't think you should do this

https://codereview.appspot.com/12636043/diff/33001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:49: func (g *Gopher) WriteToFile(f *os.File)
(int64, error) {
is this even in the slides?

[francesc, 2013-08-16 06:14:38]

PTAL

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

https://codereview.appspot.com/12636043/diff/33001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:38: var g io.WriterTo = &Gopher{
On 2013/08/16 03:17:52, adg wrote:
> this is a weird variable declaration. i don't think you should do this

Done.

https://codereview.appspot.com/12636043/diff/33001/2013/bestpractices/shorter...
2013/bestpractices/shortercode1.go:49: func (g *Gopher) WriteToFile(f *os.File)
(int64, error) {
On 2013/08/16 03:17:52, adg wrote:
> is this even in the slides?

Yes, slide about "Ask for what you need" (line 175 in bestpractices.slide)

[adg, 2013-08-19 03:47:53]

LGTM

[francesc, 2013-08-19 23:16:08]

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

go.talks: bestpractices: adding compelling example for error handling.

R=adg, r, dan.kortschak, nightlyone
CC=golang-dev
https://codereview.appspot.com/12636043