Implement lab 3 - letters #634
Conversation
jimbosoft
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe,
rokane and
samuong
as code owners
Codecov Report
@@ Coverage Diff @@
## master #634 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 274
Lines ? 5704
Branches ? 0
=======================================
Hits ? 5704
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think is all good, being such a simple exercise, I would recommend you to implement tables for testing though, the Code Owners seem to always want that method instead of the multiple tests you did. Here is explained: table driven testing
03_letters/jimbotech/letter_test.go
Outdated
| ) | ||
|
|
||
| func TestLetters(t *testing.T) { | ||
| var sentence = "marry had a little lamb" |
There was a problem hiding this comment.
nit: "mary" is spelt with one "r".
03_letters/jimbotech/letter_test.go
Outdated
| t.Errorf("Result %v differs from expected %v", res, expected) | ||
| } | ||
| } | ||
| func TestEmptySorter(t *testing.T) { |
There was a problem hiding this comment.
nitpick: blank line between functions.
03_letters/jimbotech/letter_test.go
Outdated
| } | ||
| res := sortLetters(testMap) | ||
|
|
||
| eq := reflect.DeepEqual(res, expected) |
There was a problem hiding this comment.
nitpick: idiomatic Go eschews simple variables like this, instead preferring:
if !reflect.DeepEqual(res, expected) {
Splitting your if over multiple lines by using an intermediate variable actually makes it less readable.
03_letters/jimbotech/letter_test.go
Outdated
| expected := []string{ | ||
| "\x00:0", "a:93", "ß:100000", "å:-1", | ||
| } | ||
| res := sortLetters(testMap) |
There was a problem hiding this comment.
A far more common name for res here is actual. You'll see that is very common in most tests.
03_letters/jimbotech/letter_test.go
Outdated
|
|
||
| eq := reflect.DeepEqual(res, expected) | ||
| if !eq { | ||
| t.Errorf("Result maps %v differs from expected %v", res, expected) |
There was a problem hiding this comment.
Go error strings should not start with capital letters: https://github.com/golang/go/wiki/CodeReviewComments#error-strings
please fix throughout.
03_letters/jimbotech/letter_test.go
Outdated
| } | ||
|
|
||
| func TestMain(t *testing.T) { | ||
|
|
There was a problem hiding this comment.
nit: I'm not going to go hard on what I see as unnecessary extra blank lines, but please be consistent. All your other functions do not have a blank line after the function signature; this one does.
03_letters/jimbotech/letter_test.go
Outdated
|
|
||
| func TestMain(t *testing.T) { | ||
|
|
||
| want := "a:2\nb:1\n" |
There was a problem hiding this comment.
This is usually called expected (along with actual). got and want are also fine, but be consistent. In a previous test you used expected, so I expect you to also use that here.
03_letters/jimbotech/letter_test.go
Outdated
| result := buf.String() | ||
|
|
||
| if result != want { | ||
| t.Errorf("expected %v, got %v", want, result) |
There was a problem hiding this comment.
wow. this is tortured. expected == want, got == result. just use the same name in the code as the string. i.e:
t.Errorf("want %v, got %v", want, got)
or
t.Errorf("expected %v, actual %v", expected, actual)
Either way, more consistency please.
There was a problem hiding this comment.
LGTM.
I don't know whether you noticed or not, but I can tell you that reading your tests now that you have consistent "expected" and "actual" naming, it reads quite a lot better to me. I can read it so much faster than before. It may seem like trivial nitpicking, but it does really make a difference. Thanks.
Fixes #633
Review of colleague's PR #534
Changes proposed in this PR:
-