lab 03 updated, issue fixed #457
Conversation
Marchitect
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe and
rokane
as code owners
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 40 41 +1
Lines 665 686 +21
=====================================
+ Hits 665 686 +21
Continue to review full report at Codecov.
|
I wanted to say I don't even understand my logic now lolz, thank you for the feedback though :) |
There was a problem hiding this comment.
Please add link to your review of colleague's PR and fix commit messages with git rebase -i HASH
|
|
||
| func letters(s string) map[rune]int { | ||
| m := make(map[rune]int) | ||
| // keep a list of what characters exist in the string s |
| // keep a list of what characters exist in the string s | ||
| chars := "" | ||
| for _, c := range s { | ||
| if !strings.ContainsAny(chars, string(c)) { |
There was a problem hiding this comment.
This is inefficient. ContainsAny is O(n) and so it strings.Count(s, string(c)), updating a map is O(1).
I accept that runtime is irrelevant for the size of strings expected in this program, but it is good practise to be aware.
for _, c := range s {
m[c]++
}should do the trick
|
|
||
| func sortLetters(m map[rune]int) []string { | ||
| result := make([]string, 0) | ||
| keys := make([]int, 0) |
There was a problem hiding this comment.
Add the capacity to make as you know it len(m).
| sort.Ints(keys) | ||
| for _, k := range keys { | ||
| unquoted, _ := strconv.Unquote(strconv.QuoteRune(rune(k))) | ||
| result = append(result, unquoted+":"+strconv.Itoa(m[rune(k)])) |
There was a problem hiding this comment.
s := fmt.Sprintf("%c:%d", rune(k), m[rune(k)])
result = append(result, s)| expected := sortLetters(letters(table.input)) | ||
| actual := table.output | ||
| // reference: https://yourbasic.org/golang/compare-slices/ | ||
| if !reflect.DeepEqual(actual, expected) { |
There was a problem hiding this comment.
Use testify assert.Equal to turn these three lines into a oneliner.
Fixes #447
Review of colleague's PR #
Changes proposed in this PR: