-
-
Notifications
You must be signed in to change notification settings - Fork 944
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
[go/nucleotide-count] add mentoring notes #2315
base: main
Are you sure you want to change the base?
Conversation
@IsaacG PTAL |
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.
Note I'm not a Go track maintainer
Using a map is probably the most idiomatic solution to this problem, especially since the Histogram type basically must be a map. Using a switch is about 4 times faster over the provided test cases because it does not require hashing. | ||
|
||
#### Map Example | ||
```Go |
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.
I thought language tags needed to be all lowercase to work. Is that incorrect?
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.
It looks like it's titlecase in the language file GitHub points to. Or maybe it's not case sensitive?
} | ||
``` | ||
|
||
### Common Suggestions |
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.
I think the parser needs a newline after headers. At least I believe that's the convention in other markdown files.
``` | ||
|
||
### Common Suggestions | ||
- When you have a type definition for the Histogram, you can use it like the underlying type. For instance, you can `make(Histogram, 4)` or instantiate a literal `Histogram{...}`. |
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.
for the Histogram
When your define a Histogram type?
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.
I was trying to be consistent with the specific language of Type Definitions, but that probably doesn't matter as much as it seemed like it might at the time I wrote that.
### Common Suggestions | ||
- When you have a type definition for the Histogram, you can use it like the underlying type. For instance, you can `make(Histogram, 4)` or instantiate a literal `Histogram{...}`. | ||
- If you know exactly how many elements will be in a map, it makes sense to set the capacity. | ||
- Using numeric literals (e.g. `65` for A) throughout the code to represent letters is **not** reasonable. Instead, use rune literals (e.g. `'A'`), rune literals cast to bytes (e.g. `byte('A')`), or define constants (e.g. `const A = byte('A')`). All of these can be handled at compile time and will not impact runtime performance. |
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.
One sentence per line please. Even in lists.
Co-authored-by: Isaac Good <[email protected]>
@IsaacG can you give this a second look after the changes? |
|
Add mentoring notes for Nucleotide Count exercise.