-
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
Puppy Errors #485
Puppy Errors #485
Conversation
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.
|
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.
Good work mate!
1 blocking issue.
1 suggestion
1 architectural issue to be decided on what you want to do
- 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
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.
I think some things can be simplified a bit and made a bit more idiomatic, but overall I like it. You may also have a couple of changes to incorporate from my lab 6 feedback.
} | ||
|
||
func missingPuppy(id int64) error { | ||
return &PupError{fmt.Sprintf("Puppy not found: %d", id), missingPup} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Error
with fieldsMessage
andCode
Storer
interface for all methods to also returnerror