-
-
Notifications
You must be signed in to change notification settings - Fork 945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add mentoring notes for go/pig-latin #2311
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,72 @@ | ||||||
# Pig Latin | ||||||
|
||||||
### 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The three parts/functions together form a complete solution. I can try to reorganize if you think that would help it flow better. |
||||||
|
||||||
Solutions vary considerably, but most reasonable solutions should demonstrate these three insights: | ||||||
|
||||||
1. Decoupled logic to break down a sentence into words and reform the Pig Latin sentence. | ||||||
|
||||||
``` | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are single-letter variables idiomatic Go code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
pigWords = append(pigWords, pigWord(w)) | ||||||
} | ||||||
return strings.Join(pigWords, " ") | ||||||
} | ||||||
``` | ||||||
|
||||||
2. Identify that some number of consonants (according to the rules) will be added moved to the end of the word and "ay" will be added to the end of the word. | ||||||
|
||||||
``` | ||||||
func pigWord(word string) string { | ||||||
consonants := consonantCount(word) | ||||||
return fmt.Sprintf("%s%say", word[consonants:], word[:consonants]) | ||||||
} | ||||||
``` | ||||||
|
||||||
3. Identify how many letters count as consonants. | ||||||
|
||||||
``` | ||||||
func consonantCount(word string) int { | ||||||
if strings.HasPrefix(word, "xr") || strings.HasPrefix(word, "yt") { | ||||||
return 0 | ||||||
} | ||||||
for i, r := range word { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More readable var names might be good here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
switch r { | ||||||
case 'a', 'e', 'i', 'o': | ||||||
return i | ||||||
case 'u': | ||||||
if i > 0 && word[i-1] == 'q' { | ||||||
// When a 'qu' is together, the u is also moved to the end of the word | ||||||
return i + 1 | ||||||
} | ||||||
return i | ||||||
case 'y': | ||||||
if i == 0 { | ||||||
// Leading y is not considered a vowel | ||||||
continue | ||||||
} | ||||||
return i | ||||||
} | ||||||
} | ||||||
return 0 | ||||||
} | ||||||
``` | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "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?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. | ||||||
* Reduce the number of special cases by generalizing logic. For instance, the first vowel in the word can potentially be handled the same, regardless of whether it is at the start of the word or not. | ||||||
* Prefer switch/case to long if/else chains. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this personal preference or idiomatic Go? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. | ||||||
Comment on lines
+66
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two are general comments and not specific to this exercise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I feel like that's ambiguous, otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be
##
and not###
based on a spot check of other notesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.