Puppy Errors #485
Puppy Errors #485
Conversation
nicholascross
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe and
rokane
as code owners
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 76 77 +1
Lines 1312 1385 +73
=====================================
+ Hits 1312 1385 +73
Continue to review full report at Codecov.
|
- Create an executable go program
- Copy the CRUD puppy from upstream master
- Add a custom error type `Error` with fields `Message` and `Code`
- Extend the `Storer` interface for all methods to also return `error`
- Create errors for:
- Value < 0
- ID not found in Read, Update and Delete
- Add locking for proper use of sync.Map
| } | ||
|
|
||
| func missingPuppy(id int64) error { | ||
| return &PupError{fmt.Sprintf("Puppy not found: %d", id), missingPup} |
There was a problem hiding this comment.
Error strings should not start with a capital, as they are often wrapped and this leads to capitals in the middle of the message. See https://github.com/golang/go/wiki/Errors for more details.
This applies to the other two errors too.
| } | ||
|
|
||
| func (e *PupError) Error() string { | ||
| return fmt.Sprintf("[%d] %s", e.Code, e.Message) |
There was a problem hiding this comment.
Putting the code into the formatted string doesn't add any useful information because the code is meaningless to humans. The error string contains all the useful information. I'd suggest leaving it out.
| } | ||
|
|
||
| func (ms MapStore) CreatePuppy(p Puppy) (bool, error) { | ||
| if p.Value < 0 { |
There was a problem hiding this comment.
I don't know about dogs, but I'm pretty sure my cat has negative value.
| CreatePuppy(p Puppy) (bool, error) | ||
| RetrievePuppy(int64) (*Puppy, error) | ||
| UpdatePuppy(int64, Puppy) (bool, error) | ||
| DeletePuppy(int64) (bool, error) |
There was a problem hiding this comment.
Having both a bool and error return value seems redundant. Whenever the bool is true, error is nil and vice versa. You can just return an error, and all is good if it is nil.
This applies to CreatePuppy, UpdatePuppy and DeletePuppy
| s.lock.Lock() | ||
| defer s.lock.Unlock() | ||
|
|
||
| if _, ok := s.store.Load(p.ID); ok { |
There was a problem hiding this comment.
You should be able to use LoadOrStore (https://golang.org/pkg/sync/#Map.LoadOrStore) without needing the mutex (and also simpler code):
if _, ok := s.LoadOrStore(p.ID, p); ok {
return false, puppyAlreadyExists(p.ID)
}
return true, nil
Even if this is run concurrently with one of the methods that takes the mutex, they do not require the puppy not to exist between the two sync.Map operations so CreatePuppy done this way does not interfere.
Although as I think about it, perhaps keeping the mutex in this method is better as it is clearer. However, LoadOrStore is still a shorter way to implement this.
| s.lock.Lock() | ||
| defer s.lock.Unlock() | ||
|
|
||
| if _, ok := s.store.Load(id); ok { |
There was a problem hiding this comment.
Align the happy path to the left edge (https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88) by inverting the sense of the conditional:
So,
if _, ok := s.store.Load(id); !ok {
return false, missingPuppy(id)
}
p.ID = id
s.store.Store(p.ID, p)
return true, nil
| s.lock.Lock() | ||
| defer s.lock.Unlock() | ||
|
|
||
| if _, ok := s.store.Load(id); ok { |
There was a problem hiding this comment.
Similarly, align the happy path to the left:
if _, ok := s.store.Load(id); !ok {
return false, missingPuppy(id)
}
s.store.Delete(id)
return true, nil
| type MapStore map[int64]Puppy | ||
|
|
||
| const ( | ||
| missingPup = iota |
There was a problem hiding this comment.
Using Pup in these error codes and in the error type, but using Puppy everywhere is inconsistent. It would help if you used Puppy everywhere instead of Pup.
|
|
||
| type PupError struct { | ||
| Message string | ||
| Code int |
There was a problem hiding this comment.
Rather than one error type with a code to differentiate the errors, Go tends towards defining different error types for different errors named FooError for parameterised errors, or package-level variables that are error values created with errors.New() named ErrFoo for static errors.
See https://github.com/golang/go/blob/master/src/encoding/json/encode.go#L230 for an example of the former and https://github.com/golang/go/blob/master/src/encoding/gob/decode.go#L19 for an example of the latter.
Or just create them on the fly as needed: https://github.com/golang/go/blob/master/src/database/sql/sql.go#L542
https://github.com/golang/go/wiki/Errors also has a bit more to say about this too.
I think the main thing is that your errors don't look like errors (from a Go perspective). return nil, missingPuppy(id) does not say err(or) anywhere which is what I would usually expect to see in Go code.
There was a problem hiding this comment.
Ok, so it looks like I didn't read the lab description where it says "Add a custom error type Error with fields Message and Code" - so ignore my comment about using Code (or not, as you please). I think my last paragraph in this comment still stands though - the errors don't look like errors, so if you can put "[Ee]rr" in the names somewhere I think it would read better.
|
The go-course is now closed. Thank you very much for participating. |
Fixes #484
Review of colleague's PR #478
Changes proposed in this PR:
Errorwith fieldsMessageandCodeStorerinterface for all methods to also returnerror