Index: 2014/readability.slide |
=================================================================== |
new file mode 100644 |
--- /dev/null |
+++ b/2014/readability.slide |
@@ -0,0 +1,388 @@ |
+When in Go, do as Gophers do |
+Go Conference 2014 autumn |
+30 Nov 2014 |
+ |
+Fumitoshi Ukai |
+Google Software Engineer - Chrome Infra team |
+ukai@google.com |
+https://plus.google.com/+FumitoshiUkai |
+@fumitoshi_ukai |
+ |
+* Go Readability Approver |
+ |
+A team to review Go readability. |
+ |
+- help to learn idiomatic Go though code review |
+- review code of projects that are not main project of the reviewer |
+ |
+- [[http://blogger.ukai.org/2013/12/code-readability.html][I joined the team about a year ago]] as 20% time, and reviewed ~200 CLs |
+- For now, I'm reviewing at most 3 CLs per day, 12 CLs per week. |
+ |
+.image readability/project.png |
+.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]] |
+ |
+* What is Readability skill? |
+ |
+Literacy of a programming language. |
+ |
+A skill to read or write *idiomatic* code. |
+ |
+Each programming language has its own preferred style. |
+In C++, each project chooses a preferred style. |
+ |
+- [[http://google-styleguide.googlecode.com/svn/trunk/cppguide.html][google]]([[http://www.chromium.org/developers/coding-style][chromium]]), [[https://www.webkit.org/coding/coding-style.html][webkit]]([[http://www.chromium.org/blink/coding-style][blink]]) |
+ |
+Don't write Go code as you write code in C++/Java/Python. |
+Write Go code as Gophers write code. |
+ |
+* Go code should ... |
+ |
+- be articulate, concise. |
+- provide a simple API. |
+- have precise comments. |
+- be readable, top-down code. |
+ |
+_"_Want_to_understand_something_in_google_servers?_Read_the_Go_implementation!_"_ |
+ |
+.caption by some Googler |
+ |
+.image readability/pkg.png |
+.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]] |
+ |
+ |
+* Good tools |
+ |
+[[https://golang.org/cmd/gofmt/][go fmt]] - format Go programs. |
+[[https://godoc.org/golang.org/x/tools/cmd/vet][go vet]] - report suspicious code |
+[[https://github.com/golang/lint][golint]] - report coding style errors. |
+[[http://blog.golang.org/godoc-documenting-go-code][godoc]] - browse documenation |
+ |
+# Go code is easy to read for tools too. |
+ |
+* Tools are not enough |
+ |
+# Developer doesn't read code the same as tools. |
+# Programs(compiler, etc) will read code if its syntax is ok. |
+# Tools are not enough, have false-positive/false-negative. |
+# Needs human judgments. |
+ |
+Readable code == easy to recognize, less burden for brain. |
+Both writer and reader should have readability skills. |
+Go is very simple ([[https://golang.org/ref/spec][lang spec]] is about 50 pages) |
+ |
+.image readability/gophers5th.jpg |
+.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]] |
+ |
+* Readability Reviews |
+ |
+- Any mistakes/bugs? |
+- Layout? |
+- Simple code flow? |
+- Simple API? |
+ |
+.image readability/gopher-ok-no.png |
+.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]], and [[https://github.com/tenntenn/gopher-stickers/][tenntenn]] |
+ |
+* mistakes/bugs |
+ |
+* error check |
+ |
+original code |
+ |
+.code readability/err_regexp_bad.go |
+ |
+revised |
+ |
+.code readability/err_regexp_good.go |
+ |
+- Check error with [[https://golang.org/pkg/regexp/#MustCompile][regexp.MustCompile]]. |
+- Must should be used only in [[http://golang.org/ref/spec#Package_initialization][initialization]](package `var` or `init()` ). |
+ |
+- [[http://golang.org/ref/spec#String_literals][Raw string literal]] makes it easy to read regexp. |
+ |
+* error check: original code |
+ |
+.code readability/err_close_write_bad.go |
+ |
+* error check: revised |
+ |
+.code readability/err_close_write_good.go |
+ |
+- Check error of Close for write. |
+- No need to use defer, when it's simpler. |
+ |
+* in-band error: original code |
+ |
+.code readability/in-band-error-client.go |
+ |
+.code readability/in-band-error.go |
+ |
+* return value and error: revised |
+ |
+.code readability/val-and-error.go |
+ |
+[[http://golang.org/doc/effective_go.html#multiple-returns][Return error as error, not as some value]] |
+ |
+* error design |
+ |
+If client doesn't need to distinguish errors, e.g. ok with `err` `!=` `nil` check only. |
+ |
+ fmt.Errorf("error in %s", val) or errors.New("error msg") |
+ |
+If client wants to distinguish several errors by error code. |
+ |
+ var ( |
+ ErrInternal = errors.New("foo: inetrnal error") |
+ ErrBadRequest = errors.New("foo: bad request") |
+ ) |
+ |
+If you want to put detailed information in error. |
+ |
+ type FooError struct { /* fields of error information */ } |
+ func (e *FooError) Error() string { return /* error message */ } |
+ |
+ &FooError{ /* set error data */ } |
+ |
+Don't use `panic`. |
+ |
+- But when you do, use it only within the package, and [[http://golang.org/doc/effective_go.html#recover][return error with catching it by recover]]. |
+ |
+* nil error |
+ |
+.code readability/nil_error.go |
+ |
+[[https://golang.org/doc/faq#nil_error][FAQ: Why is my nil error value not equal to nil?]] |
+ |
+[[http://blog.golang.org/laws-of-reflection][interface has 2 data]]. interface value is nil == both data is nil. |
+ |
+- value |
+- type information |
+ |
+* embed interface: original code |
+ |
+.code readability/implement-interface-bad.go |
+ |
+- `scan.Writer` is an interface. |
+- `ColumnWriter` will have methods of the `scan.Writer` interface (i.e. `ColumnWriter` implements the `scan.Writer` interface), but ... |
+ |
+* check interface implementation: revised |
+ |
+.code readability/implement-interface-good.go |
+ |
+- The original author wanted to check `ColumnWriter` [[http://golang.org/doc/effective_go.html#blank_implements][implements]] the `scan.Writer` interface. |
+ |
+* embed interface |
+ |
+If a struct doesn't have a method of a interface explicitly, the interface is embedded in the struct, and you didn't set the interface field to a concrete value (i.e. the interface field value is nil), the method call will panic. |
+ |
+.code readability/nil_interface_en.go |
+ |
+It would be useful in a test when you want to implement only a subset of methods in the huge interface. |
+ |
+* Readable layout |
+ |
+* layout of fields in struct: original code |
+ |
+.code readability/struct-field-bad.go |
+ |
+* layout of fields in struct: revised |
+ |
+.code readability/struct-field-good.go |
+ |
+- Organize fields in groups, with blank lines between them. |
+- Put [[https://golang.org/pkg/sync/#Mutex][sync.Mutex]] in top of a block of fields that the mutex protects. |
+ |
+* Long line |
+ |
+.code readability/long-line-fold.go |
+ |
+* into one line |
+ |
+.code readability/long-line-nofold.go |
+ |
+- [[https://golang.org/s/comments#Line_Length][No rigid line length limit]] |
+- though, can't we make it shorter? |
+ |
+* Choose concise names |
+ |
+[[https://golang.org/s/comments#Variable_Names][Choose good name in the context]] |
+ |
+- Long names are not always better than short names. |
+ |
+Short and accurate names. |
+ |
+- [[https://golang.org/s/comments#Package_Names][SamplingServer in sampling package is stutter]]. Name `Server`, which clients will write as `sampling.Server`. |
+- Use [[https://golang.org/s/comments#Receiver_Names][one or two letters for receiver names]]. |
+- Use short namse for parameters since type name will give some information. |
+- Use descriptive names for basic types, though. |
+- Use short names for local variables: prefer `i` to `index`, `r` to `reader`. |
+- Short names should be fine if function is not long. |
+ |
+* one line |
+ |
+.code readability/long-line-short.go |
+ |
+* top-down code |
+ |
+* conditional branch |
+ |
+- [[https://golang.org/s/comments#Indent_Error_Flow][Keep the normal code path at a minimal indentation.]] |
+ |
+original code |
+ |
+.code readability/if-else-bad.go |
+ |
+revised |
+ |
+.code readability/if-else-good.go |
+ |
+* conditional branch (2) original code |
+ |
+.code readability/resthandler.go |
+ |
+* conditional branch (2) revised |
+ |
+.code readability/resthandler-fix2.go |
+ |
+- factor out function. |
+ |
+* conditional branch (3): original code |
+ |
+.code readability/if-switch-bad.go |
+ |
+* conditional branch (3): revised |
+ |
+.code readability/if-switch-good.go |
+ |
+- use [[http://golang.org/ref/spec#Switch_statements][switch]] |
+ |
+* Simpler code |
+ |
+* time.Duration |
+ |
+Use [[https://golang.org/pkg/time/#Duration][time.Duration]] ([[https://golang.org/pkg/flag/#Duration][flag.Duration]]) rather than `int` or `float` to represent time duration. |
+ |
+original code |
+ |
+.code readability/time_duration_bad.go |
+.code readability/time_duration_bad1.go |
+.code readability/time_duration_bad2.go |
+ |
+revised |
+ |
+.code readability/time_duration_good.go |
+ |
+- Don't write unnecessary type conversion. |
+- Since [[http://blog.golang.org/constants][const is untyped]], no need to convert 30 to `time.Duration`. |
+- Don't write unnecessary comments. |
+ |
+* sync.Mutex and sync.Cond: original code |
+ |
+.code readability/close-cond-bad.go |
+ |
+* chan: revised |
+ |
+.code readability/close-cond-good.go |
+ |
+- You could use [[http://golang.org/ref/spec#Channel_types][chan]], instead of [[https://golang.org/pkg/sync/#Mutex][sync.Mutex]] and [[https://golang.org/pkg/sync/#Cond][sync.Cond]]. |
+ |
+* reflect: original code |
+ |
+.code readability/reflect-bad.go |
+ |
+* without reflect: revised |
+ |
+.code readability/reflect-good.go |
+ |
+- Don't use [[https://golang.org/pkg/reflect/][reflect]], if you know the type at compilation time. |
+ |
+* Test |
+ |
+* Test code |
+ |
+.code readability/test-pattern_en.go |
+ |
+- [[https://golang.org/s/comments#Useful_Test_Failures][Have helpful test failure messages]] |
+- Don't create yet-another assert function. Use existing programming language features. |
+- Write [[https://golang.org/pkg/testing/#hdr-Examples][an example test]] rather than writing how to use API in a doc comment. |
+ |
+.code readability/example_test.go |
+ |
+* Comment |
+ |
+* Comment |
+ |
+[[https://golang.org/s/comments#Package_Comments][Write package comment.]] Write command comment in main package. |
+[[https://golang.org/s/comments#Doc_Comments][Write comments on exported names.]] |
+[[https://golang.org/s/comments#Comment_Sentences][Doc comment should be a complete sentence that starts with the name being declared.]] |
+ |
+ // Package math provides basic constants and mathematical functions. |
+ package math |
+ |
+ // A Request represents a request to run a command. |
+ type Request struct { .. |
+ |
+ // Encode writes the JSON encoding of req to w. |
+ func Encode(w io.Writer, req *Request) { |
+ |
+Browse with [[http://blog.golang.org/godoc-documenting-go-code][godoc]] |
+ |
+ $ godoc bytes Buffer |
+ |
+ $ godoc -http=:6060 # http://localhost:6060/pkg |
+ |
+If you feel comments are unclear or hard to write concisely, reconsider your API design. |
+ |
+* API design. |
+ |
+Important to choose a good package name. |
+ |
+- e.g. package `util` is not good name, since most packages are utilities of something. |
+# split it into smaller packages, or merge it in bigger package, or wrong package boundary. |
+ |
+Make API simple. |
+ |
+- [[http://golang.org/doc/effective_go.html#multiple-returns][Use multiple returns]]. Don't use pointers as output parameters. |
+- Don't use pointer to slice, map, chan or interface. |
+- [[http://golang.org/doc/effective_go.html#multiple-returns][Return error as error]]: [[https://golang.org/s/comments#Don't_Panic][don't panic]] |
+- Implement common interfaces ([[https://golang.org/pkg/fmt/#Stringer][fmt.Stringer]], [[https://golang.org/pkg/io/#Reader][io.Reader]] and so on) if they match your code. |
+- Use interfaces for parameters. They makes it easier to test. |
+- e.g.: If a function reads from a file, use [[https://golang.org/pkg/io/#Reader][io.Reader]] as a parameter instead of [[https://golang.org/pkg/os/#File][*os.File]]. |
+- Prefer synchronous API to async API: refrain from using chan across package boundary. |
+- Clients can easily run synchronous API concurrently with goroutine and chan. |
+ |
+* To write readable code |
+ |
+* Code is communication |
+ |
+Be articulate: |
+ |
+- Choose good names. |
+- Design simple APIs. |
+- Write precise documentation. |
+- Don't be too complicated. |
+ |
+.image readability/talks.png |
+.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]] |
+ |
+* When you write code |
+ |
+Keep in mind |
+ |
+- Articulate, concise. |
+- Provide simple API. |
+- Have precise comment. |
+- Readable, top-down code. |
+ |
+* References |
+ |
+- Effective Go: [[https://golang.org/doc/effective_go.html][https://golang.org/doc/effective_go.html]] |
+- standard package: [[https://golang.org/pkg/][https://golang.org/pkg/]] |
+- Code Review Comments: [[https://golang.org/s/comments][https://golang.org/s/comments]] |
+ |
+- Go for gophers: [[http://talks.golang.org/2014/go4gophers.slide][http://talks.golang.org/2014/go4gophers.slide]] |
+- What's in a name? [[http://talks.golang.org/2014/names.slide][http://talks.golang.org/2014/names.slide]] |
+- Organizing Go code: [[http://talks.golang.org/2014/organizeio.slide][http://talks.golang.org/2014/organizeio.slide]] |
+ |
+.image readability/ref.png |
+.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]] |