-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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 C++, Python, Go code for chapter_hashing #110
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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.
Thanks for the code! Please address the comment.
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.
Hi @msk397 , there are a few comments to be addressed. Thanks!
By the way, it is recommended to press |
Sorry, this is my first time participating in an open source project, there are a lot of deficiencies.🥲 Thank you for your code review! I will fix these mistakes. |
Not a problem! Unfamiliar at first but you get used to it. |
codes/go/pkg/print_utils.go
Outdated
@@ -96,3 +96,10 @@ func showTrunk(t *trunk) { | |||
showTrunk(t.prev) | |||
fmt.Print(t.str) | |||
} | |||
|
|||
// PrintHashMap Print a hash map | |||
func PrintMap(m map[int]string) { |
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.
Please implement a function with Generics, ie., we should not limit the type of key and value.
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 added PrintMap by reference to PrintSlice function. If Generics are required, does PrintSlice function also need to be supported?
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.
Sorry that I missed this comment. Yes, I think it should also support generics. Could you help update it? Thanks!
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.
No problem! @krahets
I add a comment, please address it, thanks! I'm wondering which languages you mainly write in. It is recommended to focus on working on the language you are most familiar with. |
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.
Thanks @msk397 for the great work! Now merge it.
I will focus on my main language. Thanks for your advice! |
Yes, or push the code in one language at first. And you can fix other languages following the comments. After merging, push other languages. |
No description provided.