|
|
Descriptiongo.talks: add "What's in a name?" slides
These slide were presented at the Go meetup in Paris, October 2014.
Patch Set 1 #
Total comments: 2
Patch Set 2 : diff -r 05fbbfe364cfd6d409a4dd43229895d95cac2d4d https://code.google.com/p/go.talks #
Total comments: 26
Patch Set 3 : diff -r 05fbbfe364cfd6d409a4dd43229895d95cac2d4d https://code.google.com/p/go.talks #
Total comments: 3
Patch Set 4 : diff -r 05fbbfe364cfd6d409a4dd43229895d95cac2d4d https://code.google.com/p/go.talks #Patch Set 5 : diff -r 05fbbfe364cfd6d409a4dd43229895d95cac2d4d https://code.google.com/p/go.talks #Patch Set 6 : diff -r 05fbbfe364cfd6d409a4dd43229895d95cac2d4d https://code.google.com/p/go.talks #Patch Set 7 : diff -r 98aa52306e414181d79514615359009943ac64e0 https://code.google.com/p/go.talks #
Total comments: 1
MessagesTotal messages: 19
Hello r (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.talks
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/1/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/1/2014/names.slide#newcode208 2014/names.slide:208: The standard library is a great place to fidn good Go code. s/fidn/find/
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/1/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/1/2014/names.slide#newcode208 2014/names.slide:208: The standard library is a great place to fidn good Go code. On 2014/10/13 06:25:31, kortschak wrote: > s/fidn/find/ Done.
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode189 2014/names.slide:189: * Import paths Suggestion: Explicitly discourage also some/all of - Any punctuation in the import path basename. - Use of "go" suffix or prefix where not necessary ("gowhatever" no, but e.g. "gowellknowncommand" might be ok for disambiguation). - Use of upper case letters in import paths - doesn't work properly across file systems. "A" and "a" can then be the same thing even when not.
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode122 2014/names.slide:122: Return values should only be named for documentation purposes. What if I name a return value to avoid typing a long type twice and also save a line on declaration? E.g.: func foo() (res [][]string) { ... vs func foo() [][]string { var res [][]string ...
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode63 2014/names.slide:63: for _, commit := range r.commits { I think r.commits, r.lastSeen, should be repo.commits, repo.lastSeen (in the Bad example)
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode189 2014/names.slide:189: * Import paths On 2014/10/13 07:00:40, 0xjnml wrote: > - Any punctuation in the import path basename. s/punctuation/punctuation or spaces/
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode148 2014/names.slide:148: Remember this when naming exported variables, constnats, functions, and types. s/constnats/constants/
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode16 2014/names.slide:16: Readability is a defining quality of good code. s/a/the/ https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode82 2014/names.slide:82: func (r *Repo) findBranches() error { i don't find this especially better https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode113 2014/names.slide:113: And longer where the types are less descriptive: not grammatical. this is supposed to follow the sentence above but it's broken https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode122 2014/names.slide:122: Return values should only be named for documentation purposes. return values should be used to return a value. what are you saying? do you mean named return values? i believe they are unfairly castigated, but i will not fight you on this. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode128 2014/names.slide:128: Don't use them gratuitously; it just adds stutter. bad grammar (them/it) plus i what stutter do you mean? the only stutter above is 'err error' and it's not bad. they don't just add bad things; they also add documentation poor slide. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode135 2014/names.slide:135: By convention, they are one or two characters that reflect the receiver type: why? because they will appear on almost every line. here and generally: nothing about consistency. if you use r here, don't use rdr in a different method https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode156 2014/names.slide:156: Interfaces that specify just one function are usually just that function name with 'er' appended to it. s/function/method/g what about when there are multiple methods? https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode166 2014/names.slide:166: } and sometimes we use english to make it nicer: ByteReader not ReadByter
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode16 2014/names.slide:16: Readability is a defining quality of good code. On 2014/10/13 16:43:20, r wrote: > s/a/the/ Done. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode63 2014/names.slide:63: for _, commit := range r.commits { On 2014/10/13 07:28:57, h wrote: > I think r.commits, r.lastSeen, should be repo.commits, repo.lastSeen (in the Bad > example) Done. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode82 2014/names.slide:82: func (r *Repo) findBranches() error { On 2014/10/13 16:43:20, r wrote: > i don't find this especially better It works better with an explanation. I agree it's not the best example, though. I'll try to find another. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode113 2014/names.slide:113: And longer where the types are less descriptive: On 2014/10/13 16:43:20, r wrote: > not grammatical. this is supposed to follow the sentence above but it's broken Done. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode122 2014/names.slide:122: Return values should only be named for documentation purposes. On 2014/10/13 16:43:20, r wrote: > return values should be used to return a value. what are you saying? do you mean > named return values? It says "Return values should only be *named* for documentation purposes." Did you misread or are you making a subtler point? https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode122 2014/names.slide:122: Return values should only be named for documentation purposes. On 2014/10/13 07:07:19, dvyukov wrote: > What if I name a return value to avoid typing a long type twice and also save a > line on declaration? I think this is bad style when used for exported functions. The "res" adds noise to the function signature. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode128 2014/names.slide:128: Don't use them gratuitously; it just adds stutter. On 2014/10/13 16:43:20, r wrote: > bad grammar (them/it) plus i what stutter do you mean? the only stutter above is > 'err error' and it's not bad. > they don't just add bad things; they also add documentation > > poor slide. > I made it clear that these are supposed to be good examples of named return values. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode135 2014/names.slide:135: By convention, they are one or two characters that reflect the receiver type: On 2014/10/13 16:43:20, r wrote: > why? because they will appear on almost every line. > > here and generally: nothing about consistency. if you use r here, don't use rdr > in a different method Done. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode148 2014/names.slide:148: Remember this when naming exported variables, constnats, functions, and types. On 2014/10/13 07:52:41, 0intro wrote: > s/constnats/constants/ Done. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode156 2014/names.slide:156: Interfaces that specify just one function are usually just that function name with 'er' appended to it. On 2014/10/13 16:43:20, r wrote: > s/function/method/g > > what about when there are multiple methods? Done. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode166 2014/names.slide:166: } On 2014/10/13 16:43:20, r wrote: > and sometimes we use english to make it nicer: ByteReader not ReadByter Done. https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode189 2014/names.slide:189: * Import paths On 2014/10/13 07:00:40, 0xjnml wrote: > Suggestion: Explicitly discourage also some/all of > > - Any punctuation in the import path basename. > - Use of "go" suffix or prefix where not necessary ("gowhatever" no, but e.g. > "gowellknowncommand" might be ok for disambiguation). > - Use of upper case letters in import paths - doesn't work properly across file > systems. "A" and "a" can then be the same thing even when not. Done.
Sign in to reply to this message.
looks good but i think your short names example is very weak, even counterproductive https://codereview.appspot.com/156140043/diff/40001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/40001/2014/names.slide#newcode77 2014/names.slide:77: return nil can you find an example where the names are a larger fraction of the space? https://codereview.appspot.com/156140043/diff/40001/2014/names.slide#newcode102 2014/names.slide:102: } i still prefer the original in this case.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/156140043/diff/40001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/40001/2014/names.slide#newcode77 2014/names.slide:77: return nil On 2014/10/14 17:53:31, r wrote: > can you find an example where the names are a larger fraction of the space? I went with utf8.RuneCount, take a look.
Sign in to reply to this message.
https://codereview.appspot.com/156140043/diff/20001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/20001/2014/names.slide#newcode122 2014/names.slide:122: Return values should only be named for documentation purposes. On 2014/10/14 16:53:13, adg wrote: > On 2014/10/13 07:07:19, dvyukov wrote: > > What if I name a return value to avoid typing a long type twice and also save > a > > line on declaration? > > I think this is bad style when used for exported functions. The "res" adds noise > to the function signature. Maybe I mis-understand your sentence. I read is as -- you must use named return values only if you want the name to serve as documentation for users (or to reference it from a comment). Is it correct? If so, my question is -- what if I want to use it merely to not declare the variable in the function body? let's say it's unexported function.
Sign in to reply to this message.
On 15 October 2014 14:25, <dvyukov@google.com> wrote: > I read is as -- you must use named return values only if you want the > name to serve as documentation for users (or to reference it from a > comment). > Is it correct? > Yes. > If so, my question is -- what if I want to use it merely to not declare > the variable in the function body? let's say it's unexported function. > I think that's fine. I changed the sentence to explicitly call out exported functions.
Sign in to reply to this message.
On Wed, Oct 15, 2014 at 5:55 PM, Andrew Gerrand <adg@golang.org> wrote: > > On 15 October 2014 14:25, <dvyukov@google.com> wrote: >> >> I read is as -- you must use named return values only if you want the >> name to serve as documentation for users (or to reference it from a >> comment). >> Is it correct? > > > Yes. > >> >> If so, my question is -- what if I want to use it merely to not declare >> the variable in the function body? let's say it's unexported function. > > > I think that's fine. I changed the sentence to explicitly call out exported > functions. ok thanks
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=b153cff0b0fc&repo=talks *** go.talks: add "What's in a name?" slides These slide were presented at the Go meetup in Paris, October 2014. LGTM=r R=r, dan.kortschak, 0xjnml, dvyukov, hariharan.uno, 0intro CC=golang-codereviews https://codereview.appspot.com/156140043
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/156140043/diff/120001/2014/names.slide File 2014/names.slide (right): https://codereview.appspot.com/156140043/diff/120001/2014/names.slide#newcode78 2014/names.slide:78: func RuneCount(b []byte) int { p and b used in this function interchangeably.
Sign in to reply to this message.
Yeah I noticed that before delivering it tonight https://codereview.appspot.com/154370044 On 15 October 2014 23:32, <dan.kortschak@adelaide.edu.au> wrote: > > https://codereview.appspot.com/156140043/diff/120001/2014/names.slide > File 2014/names.slide (right): > > https://codereview.appspot.com/156140043/diff/120001/ > 2014/names.slide#newcode78 > 2014/names.slide:78: func RuneCount(b []byte) int { > p and b used in this function interchangeably. > > https://codereview.appspot.com/156140043/ >
Sign in to reply to this message.
|