-
Notifications
You must be signed in to change notification settings - Fork 164
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 lab 4 - Numeronym #640
Conversation
Codecov Report
@@ Coverage Diff @@
## master #640 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 280 281 +1
Lines 5754 5768 +14
=====================================
+ Hits 5754 5768 +14
Continue to review full report at Codecov.
|
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 I didn't get to this earlier, Alex. Please make sure you mark your PRs as "ready for review" if they still need a review. I do sometimes remember to look at [-label:"ready for review" label:"approved by colleague"] which is when I saw this one.
A couple of things to fix up here, but otherwise a good compact solution. You have made the classic mistake with this lab, which is good because you'll remember from now how exactly strings work WRT runes and bytes.
Oh, also please fill out the issue you've linked to this PR. Have a read of https://github.com/anz-bank/go-course/wiki/Software-Engineering-with-Go for why we want this extra stuff. |
Changes requested implemented. Golang-CI is acting weird. |
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.
lgtm
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.
There is a bug in here which suggests some more test cases are needed.
04_numeronym/alextmz/main.go
Outdated
} | ||
|
||
func numeronym(s string) string { | ||
if len(s) < 4 { |
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 exhibits a similar problem mentioned in my earlier review, and is not doing what you think it's doing. See my comment on the test cases for some examples of where this will fail.
As described in https://blog.golang.org/strings, a string is effectively a read-only slice of bytes, not runes, so the length of a string is not the number of runes, but the number of bytes.
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.
Yep, I corrected this - the len
usage was quite naive and the source of the issue.
04_numeronym/alextmz/main.go
Outdated
} | ||
|
||
func numeronym(s string) string { | ||
if utf8.RuneCountInString(s) < 4 { |
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.
RuneCountInString not declared by package utf8 (from typecheck
)
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 looks like it works now.
I see you are using both RuneCountInString
and RuneCount
. This will possibly cause confusion as it is an unnecessary distinction. When I see something like that, I wonder "why?" and when there's no reason, it just makes it harder to understand.
Also, there's no need to call it twice. If you move the numrunes := ...
to the top of the function, you can have your test be if numrunes < 4
.
But this is general software engineering practices and not related directly to Go, so I'll approve this.
Thanks for your perseverance.
Added the suggested changes... yes it looked too ugly the way it was, I should had paid more attention and not only done a quick-fix. Hey it's a software engineering course in Go, not programming in 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.
LGTM
Add lab 4 - Numeronym
Fixes #639
Review of colleague's PR #461
Changes proposed in this PR: