code review 110520043: go-tour: add defer slides

go-tour: add defer slides

[francesc, 2014-07-11 22:22:10]

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

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

[adg, 2014-07-14 06:41:40]

I'm a bit nervous about this one. Maybe it should be introduced in concert with
some idiomatic use cases; such as closing a file after reading.

https://codereview.appspot.com/110520043/diff/60001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/60001/content/flowcontrol.artic...
content/flowcontrol.article:113: A "defer" statement invokes a function whose
execution is deferred to the moment
A defer statement defers the execution of a function until the surrounding
function returns.

This code prints "hello" and then "world":

[campoy, 2014-07-14 21:01:50]

I think it's worth having defer as part of the introductory tour, and so far we
don't introduce any other package (except for net/http).

If you come up with a better example for the code showing what's the value of
defer I'm happy to change it.

https://codereview.appspot.com/110520043/diff/60001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/60001/content/flowcontrol.artic...
content/flowcontrol.article:113: A "defer" statement invokes a function whose
execution is deferred to the moment
On 2014/07/14 06:41:40, adg wrote:
> A defer statement defers the execution of a function until the surrounding
> function returns.
> 
> This code prints "hello" and then "world":
> 

Done.

[adg, 2014-07-15 02:53:35]

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:111: * Defer
link to http://blog.golang.org/defer-panic-and-recover

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:123: The parameters to the function call are
evaluated per usual, and only the actual
s/parameters/arguments/ :)

Maybe this example should be

func greet() {
  defer fmt.Println("wo" + "rld")
  fmt.Print("hello ")
}

so that we can say

"The deferred call's arguments are evaluated immediately ("wo" + "rld" is
evaluated to "world"), but the function is not executed until the greet function
returns."

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:126: The code on the left shows how the value passed
to `fmt.Printf` is evaluated
the code on the left should speak for itself. I'd prefer to drop these
sentences.

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:134: Deferred function calls are stacked. Therefore
when multiple function calls
Deferred function calls are pushed onto a stack. When a function returns, its
deferred calls are executed in last-in-first-out order.

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer-m...
File content/prog/tour/defer-multi.go (right):

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer-m...
content/prog/tour/defer-multi.go:8: // This will be executed at the end of main
in reverse order.
unnecessary comment; delete

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer.go
File content/prog/tour/defer.go (right):

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer.g...
content/prog/tour/defer.go:5: func execute(task string) {
a clearer example might be:

func count(n int) {
  fmt.Println("counting")
  for i := 0; i < n; i++ {
    defer fmt.Println(i)
  }
  fmt.Println("done")
}

but you might consider using the examples from this blog post:
http://blog.golang.org/defer-panic-and-recover

[campoy, 2014-07-15 21:50:42]

PTAL

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:111: * Defer
On 2014/07/15 02:53:35, adg wrote:
> link to http://blog.golang.org/defer-panic-and-recover

Done at the end of the section on defer.

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:123: The parameters to the function call are
evaluated per usual, and only the actual
On 2014/07/15 02:53:35, adg wrote:
> s/parameters/arguments/ :)
> 
> Maybe this example should be
> 
> func greet() {
>   defer fmt.Println("wo" + "rld")
>   fmt.Print("hello ")
> }
> 
> so that we can say
> 
> "The deferred call's arguments are evaluated immediately ("wo" + "rld" is
> evaluated to "world"), but the function is not executed until the greet
function
> returns."

Done.

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:126: The code on the left shows how the value passed
to `fmt.Printf` is evaluated
On 2014/07/15 02:53:35, adg wrote:
> the code on the left should speak for itself. I'd prefer to drop these
> sentences.

Done.

https://codereview.appspot.com/110520043/diff/80001/content/flowcontrol.artic...
content/flowcontrol.article:134: Deferred function calls are stacked. Therefore
when multiple function calls
On 2014/07/15 02:53:35, adg wrote:
> Deferred function calls are pushed onto a stack. When a function returns, its
> deferred calls are executed in last-in-first-out order.

Done.

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer-m...
File content/prog/tour/defer-multi.go (right):

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer-m...
content/prog/tour/defer-multi.go:8: // This will be executed at the end of main
in reverse order.
On 2014/07/15 02:53:35, adg wrote:
> unnecessary comment; delete

Done.

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer.go
File content/prog/tour/defer.go (right):

https://codereview.appspot.com/110520043/diff/80001/content/prog/tour/defer.g...
content/prog/tour/defer.go:5: func execute(task string) {
On 2014/07/15 02:53:35, adg wrote:
> a clearer example might be:
> 
> func count(n int) {
>   fmt.Println("counting")
>   for i := 0; i < n; i++ {
>     defer fmt.Println(i)
>   }
>   fmt.Println("done")
> }
> 
> but you might consider using the examples from this blog post:
> http://blog.golang.org/defer-panic-and-recover

I used that example for defer-multi and created a new simpler one for this.

[adg, 2014-07-16 04:52:47]

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.arti...
content/flowcontrol.article:137: f := os.Open("file")
It's weird showing this code without errors and error handling.
Maybe it should be something abstract (as it was earlier?)
like a for loop?

Maybe we don't actually need a code sample in this slide.

[campoy, 2014-07-16 18:38:45]

PTAL

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.arti...
content/flowcontrol.article:137: f := os.Open("file")
On 2014/07/16 04:52:47, adg wrote:
> It's weird showing this code without errors and error handling.
> Maybe it should be something abstract (as it was earlier?)
> like a for loop?
> 
> Maybe we don't actually need a code sample in this slide.

The for loop adds multiple deferred calls, which are presented in the next
slide. And handling errors requires knowledge of the error interface.

What about:

func readUserName() string { return "johndoe" }

func main() {
	user := readUserName()
	defer fmt.Println("bye,", user)

	if !isAuthorized(user) {
		fmt.Println(user, "is not authorized")
	}

	fmt.Println("hi, ", user)
}

"bye, user" will be printed at the end of the function regardless of the
authorization step.

[adg, 2014-07-16 23:50:42]

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.arti...
content/flowcontrol.article:137: f := os.Open("file")
On 2014/07/16 18:38:45, campoy wrote:
> On 2014/07/16 04:52:47, adg wrote:
> > It's weird showing this code without errors and error handling.
> > Maybe it should be something abstract (as it was earlier?)
> > like a for loop?
> > 
> > Maybe we don't actually need a code sample in this slide.
> 
> The for loop adds multiple deferred calls, which are presented in the next
> slide. And handling errors requires knowledge of the error interface.
> 
> What about:
> 
> func readUserName() string { return "johndoe" }
> 
> func main() {
> 	user := readUserName()
> 	defer fmt.Println("bye,", user)
> 
> 	if !isAuthorized(user) {
> 		fmt.Println(user, "is not authorized")
> 	}
> 
> 	fmt.Println("hi, ", user)
> }
> 
> "bye, user" will be printed at the end of the function regardless of the
> authorization step.

I suggest just dropping the code sample on this slide entirely. Let the .play
code example speak for itself.

[campoy, 2014-07-17 22:42:34]

PTAL

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/100001/content/flowcontrol.arti...
content/flowcontrol.article:137: f := os.Open("file")
On 2014/07/16 23:50:42, adg wrote:
> On 2014/07/16 18:38:45, campoy wrote:
> > On 2014/07/16 04:52:47, adg wrote:
> > > It's weird showing this code without errors and error handling.
> > > Maybe it should be something abstract (as it was earlier?)
> > > like a for loop?
> > > 
> > > Maybe we don't actually need a code sample in this slide.
> > 
> > The for loop adds multiple deferred calls, which are presented in the next
> > slide. And handling errors requires knowledge of the error interface.
> > 
> > What about:
> > 
> > func readUserName() string { return "johndoe" }
> > 
> > func main() {
> > 	user := readUserName()
> > 	defer fmt.Println("bye,", user)
> > 
> > 	if !isAuthorized(user) {
> > 		fmt.Println(user, "is not authorized")
> > 	}
> > 
> > 	fmt.Println("hi, ", user)
> > }
> > 
> > "bye, user" will be printed at the end of the function regardless of the
> > authorization step.
> 
> I suggest just dropping the code sample on this slide entirely. Let the .play
> code example speak for itself.

Done.

[adg, 2014-07-18 00:11:54]

Sorry for my vacillations on this cl.

https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.arti...
content/flowcontrol.article:116: The deferred call's arguments are evaluated
immeditaly, but the function call
immediately

https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer.go
File content/prog/tour/defer.go (right):

https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer....
content/prog/tour/defer.go:12: func main() {
I think this should be just a really trivial example. The point about evaluating
arguments is made clear by the next slide. Just go back to

defer fmt.Println("world")
fmt.Println("hello")

[campoy, 2014-07-18 00:36:36]

https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.arti...
content/flowcontrol.article:116: The deferred call's arguments are evaluated
immeditaly, but the function call
On 2014/07/18 00:11:53, adg wrote:
> immediately

immeditaly is a beautiful country.
done!

https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer.go
File content/prog/tour/defer.go (right):

https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer....
content/prog/tour/defer.go:12: func main() {
On 2014/07/18 00:11:53, adg wrote:
> I think this should be just a really trivial example. The point about
evaluating
> arguments is made clear by the next slide. Just go back to
> 
> defer fmt.Println("world")
> fmt.Println("hello")

This doesn't show the point about arguments being evaluated immediately though

[adg, 2014-07-18 00:48:17]

On 2014/07/18 00:36:36, campoy wrote:
>
https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.article
> File content/flowcontrol.article (right):
> 
>
https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.arti...
> content/flowcontrol.article:116: The deferred call's arguments are evaluated
> immeditaly, but the function call
> On 2014/07/18 00:11:53, adg wrote:
> > immediately
> 
> immeditaly is a beautiful country.
> done!
> 
>
https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer.go
> File content/prog/tour/defer.go (right):
> 
>
https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer....
> content/prog/tour/defer.go:12: func main() {
> On 2014/07/18 00:11:53, adg wrote:
> > I think this should be just a really trivial example. The point about
> evaluating
> > arguments is made clear by the next slide. Just go back to
> > 
> > defer fmt.Println("world")
> > fmt.Println("hello")
> 
> This doesn't show the point about arguments being evaluated immediately though

As I said, I think it's made clear by the next slide. I don't think it's
important to make all the points at once.

[campoy, 2014-07-18 00:55:51]

On 2014/07/18 00:48:17, adg wrote:
> On 2014/07/18 00:36:36, campoy wrote:
> >
>
https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.article
> > File content/flowcontrol.article (right):
> > 
> >
>
https://codereview.appspot.com/110520043/diff/120001/content/flowcontrol.arti...
> > content/flowcontrol.article:116: The deferred call's arguments are evaluated
> > immeditaly, but the function call
> > On 2014/07/18 00:11:53, adg wrote:
> > > immediately
> > 
> > immeditaly is a beautiful country.
> > done!
> > 
> >
>
https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer.go
> > File content/prog/tour/defer.go (right):
> > 
> >
>
https://codereview.appspot.com/110520043/diff/120001/content/prog/tour/defer....
> > content/prog/tour/defer.go:12: func main() {
> > On 2014/07/18 00:11:53, adg wrote:
> > > I think this should be just a really trivial example. The point about
> > evaluating
> > > arguments is made clear by the next slide. Just go back to
> > > 
> > > defer fmt.Println("world")
> > > fmt.Println("hello")
> > 
> > This doesn't show the point about arguments being evaluated immediately
though
> 
> As I said, I think it's made clear by the next slide. I don't think it's
> important to make all the points at once.

Agreed and done

[adg, 2014-07-18 01:23:58]

https://codereview.appspot.com/110520043/diff/160001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/160001/content/flowcontrol.arti...
content/flowcontrol.article:129: f := os.Open("file")
Ach, my earlier comment musn't have made it through.

os.Open returns two values, and users of the tour haven't seen it yet. Also
"close" is a built-in for closing channels. This is going to be really confusing
to newcomers.
I think we should just remove this example as well. The actual code sample
demonstrates it clearly.

[campoy, 2014-07-18 21:16:10]

https://codereview.appspot.com/110520043/diff/160001/content/flowcontrol.article
File content/flowcontrol.article (right):

https://codereview.appspot.com/110520043/diff/160001/content/flowcontrol.arti...
content/flowcontrol.article:129: f := os.Open("file")
On 2014/07/18 01:23:58, adg wrote:
> Ach, my earlier comment musn't have made it through.
> 
> os.Open returns two values, and users of the tour haven't seen it yet. Also
> "close" is a built-in for closing channels. This is going to be really
confusing
> to newcomers.
> I think we should just remove this example as well. The actual code sample
> demonstrates it clearly.

Done.

[adg, 2014-07-18 21:37:15]

LGTM

On Saturday, 19 July 2014, <campoy@google.com> wrote:

>
> https://codereview.appspot.com/110520043/diff/160001/
> content/flowcontrol.article
> File content/flowcontrol.article (right):
>
> https://codereview.appspot.com/110520043/diff/160001/
> content/flowcontrol.article#newcode129
> content/flowcontrol.article:129: f := os.Open("file")
> On 2014/07/18 01:23:58, adg wrote:
>
>> Ach, my earlier comment musn't have made it through.
>>
>
>  os.Open returns two values, and users of the tour haven't seen it yet.
>>
> Also
>
>> "close" is a built-in for closing channels. This is going to be really
>>
> confusing
>
>> to newcomers.
>> I think we should just remove this example as well. The actual code
>>
> sample
>
>> demonstrates it clearly.
>>
>
> Done.
>
> https://codereview.appspot.com/110520043/
>

[adg, 2014-07-23 03:42:29]

*** Submitted as https://code.google.com/p/go-tour/source/detail?r=ff9df8631195
***

go-tour: add defer slides

LGTM=adg
R=adg, campoy
CC=golang-codereviews
https://codereview.appspot.com/110520043

Committer: Andrew Gerrand <adg@golang.org>