Lab6 - Puppy Storer - alfredxiao #476
Conversation
alfredxiao
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe and
rokane
as code owners
Codecov Report
@@ Coverage Diff @@
## master #476 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 107 110 +3
Lines 1861 1923 +62
=====================================
+ Hits 1861 1923 +62
Continue to review full report at Codecov.
|
06_puppy/alfredxiao/store_test.go
Outdated
| t := s.T() | ||
| _ = s.store.CreatePuppy(Puppy{ID: "3", Colour: "Blue"}) | ||
| p, err := s.store.ReadPuppy("3") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Why the mix of require and assert?
There was a problem hiding this comment.
require is for not continuing with other assertions when it fails, assertions do not have this attribute. In this case, my thought was that if err is Error then there is no point in continuing.
| "sync" | ||
| ) | ||
|
|
||
| type syncStore struct { |
There was a problem hiding this comment.
Interesting choice to make this private.
There was a problem hiding this comment.
Don't make it private :)
alfredxiao
changed the title
There was a problem hiding this comment.
- Limit the number of commits to one or two
- Fix commit messages - Capitalise first word in commit message.
If you need help with this, please reach out and/or come over.
You'll also have to add a mutex to add agains race condition in the syncmap implementation (the point of using sync.Map is to protect your code from race conditions - make it "thread/goroutine safe").
Other than that looks pretty solid, well done.
| data map[string]Puppy | ||
| } | ||
|
|
||
| func (s *mapStore) CreatePuppy(p Puppy) string { |
There was a problem hiding this comment.
Use godoc for all public functions.
| } | ||
|
|
||
| func (s *mapStore) UpdatePuppy(id string, p Puppy) error { | ||
| if id != p.ID { |
There was a problem hiding this comment.
not sure why you'd have separate id field?
| } | ||
|
|
||
| _, ok := s.data[id] | ||
| if !ok { |
There was a problem hiding this comment.
if _, ok := s.data[id]; !ok {
same below.
| "sync" | ||
| ) | ||
|
|
||
| type syncStore struct { |
There was a problem hiding this comment.
Don't make it private :)
| } | ||
|
|
||
| func (s *syncStore) CreatePuppy(p Puppy) string { | ||
| id := uuid.New().String() |
| return fmt.Errorf("puppy with ID[%s] does not exists", p.ID) | ||
| } | ||
|
|
||
| s.data.Store(id, p) |
Fixes #475
Review of colleague's PR #436
Changes proposed in this PR:
Storerusing mapStorerusing sync.map