add mentoring notes for go/pig-latin #2311
Conversation
| @@ -0,0 +1,72 @@ | |||
| # Pig Latin | |||
|
|
|||
| ### Reasonable solutions: | |||
There was a problem hiding this comment.
This should probably be ## and not ### based on a spot check of other notes
There was a problem hiding this comment.
Yeah, the other notes in the Go track seem to disagree with the README for the repo, which suggests ### for headings. I wasn't sure which to follow.
| func Sentence(sentence string) string { | ||
| words := strings.Fields(sentence) | ||
| pigWords := make([]string, 0, len(words)) | ||
| for _, w := range words { |
There was a problem hiding this comment.
Are single-letter variables idiomatic Go code?
There was a problem hiding this comment.
What my seniors says is, it is idiomatic for short lived variables and loops (in small snippets as well). However, for variables with broader scopes and long lifetimes, descriptive names are appreciated.
There was a problem hiding this comment.
The greater the distance between a name's declaration and its uses, the longer the name should be. In this case, it is scoped to just a couple lines, so I consider this idiomatic.
| if strings.HasPrefix(word, "xr") || strings.HasPrefix(word, "yt") { | ||
| return 0 | ||
| } | ||
| for i, r := range word { |
There was a problem hiding this comment.
More readable var names might be good here.
There was a problem hiding this comment.
i is a well-accepted name for an index variable.
r is short for rune, and rune is a keyword. I'm open to suggestions here, but I wouldn't question this in code review, myself.
| } | ||
| ``` | ||
|
|
||
| It is possible to combine parts 2 and 3, but this often leads to less readable code that repeats parts of the logic to form the pig latin word. |
There was a problem hiding this comment.
"often" sounds subjective. Why not leave it up to the mentors to notice if the code is readable or not and to call out repeated code?
| It is possible to combine parts 2 and 3, but this often leads to less readable code that repeats parts of the logic to form the pig latin word. | |
| It is possible to combine parts 2 and 3, though sometimes doing so is hard. |
There was a problem hiding this comment.
I think it's ok to leave subjective suggestions for mentors in these notes. But maybe it needs a little more explanation about why the code is less readable?
There was a problem hiding this comment.
Most of the community solutions I saw that count consonants inline end up with more deeply logic and many returns at various levels of organization. It's not wrong and there may be marginal performance benefits, but it does reduce Clarity, which is the most important style goal of code, according to Google's Style Guide.
| ### Common suggestions: | ||
| * Avoid complex, deeply nested logic. | ||
| * Are all of the conditions treated as special cases in code actually special cases? For instance, does a leading vowel need to be treated differently than a first vowel in the middle of the word? | ||
| * Prefer switch/case to long if/else chains. |
There was a problem hiding this comment.
Is this personal preference or idiomatic Go?
There was a problem hiding this comment.
This is normative, and has associated linter rules. For instance:
| * Add explanatory comments when the intent of the code might not be clear to the reader. | ||
| * `gofmt` ensures consistent formatting, and is standard across most of the Go community. |
There was a problem hiding this comment.
These two are general comments and not specific to this exercise.
There was a problem hiding this comment.
The note about adding comments is more likely to come up in this exercise than many others, because the logic is actually somewhat complex here.
Formatting is general, but comes up frequently for me. There are similar notes on other exercises on this track. I'm not sure what the right call is here.
| ### Talking points | ||
| * `rune` vs `byte` and why iterating over a string with `range` returns runes: [Strings, bytes, runes and characters in Go](https://blog.golang.org/strings) | ||
| * The test cases do not include uppercase letters. How would the student's solution have to change to handle uppercase letters? | ||
| * `fmt.Sprintf` and `strings.Builder` are more common than using the `+` operator to concatenate strings. |
There was a problem hiding this comment.
| * `fmt.Sprintf` and `strings.Builder` are more common than using the `+` operator to concatenate strings. | |
| * `strings.Builder` is the preferred tool for building large strings. `fmt.Sprint` can be useful in place of string concatenation for shorter strings. |
There was a problem hiding this comment.
I'm not too familiar with using suggested changes in GitHub, but can you modify the suggest to clarify "in place of string concatenation with the + operator"?
I feel like that's ambiguous, otherwise.
|
|
||
| ### Reasonable solutions: | ||
|
|
||
| There is a (somewhat) reasonable solution using regular expressions, however most students are likely to use a combination of `strings` funtions or string slicing to solve this problem. |
There was a problem hiding this comment.
This section has a bunch of components but not a complete solution. You may want to present a complete solution here. If you want to explain it, you can use functions and comments and delve into details in following sections.
There was a problem hiding this comment.
The three parts/functions together form a complete solution. I can try to reorganize if you think that would help it flow better.
|
@bernot-dev Thank you for this! :) |
Co-authored-by: Isaac Good <[email protected]>
Co-authored-by: Isaac Good <[email protected]>
Add some mentoring notes for the Pig Latin exercise.