Lab7- Errors #536
Lab7- Errors #536
Conversation
mohitnag
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe and
rokane
as code owners
|
Don't forget to apply the correct labels on your PR. Also look at the PR markdown formatting - it's a little all over the place. You could be more descriptive in the error handling that's been applied. You have also included your lab 06 commit within this PR. This should be removed. It would also be handy to split the initial c+p from Lab 06 into a separate commit so Lab 07's additions can be seen explicitly. |
06_puppy/mohitnag/types.go
Outdated
| } | ||
|
|
||
| // MemStore stores Puppy details with Puppy Id as Key and Puppy as value | ||
| type MemStore map[uint32]Puppy |
There was a problem hiding this comment.
This should be called MapStore
There was a problem hiding this comment.
This will need to be updated in Lab 07 as well.
There was a problem hiding this comment.
although naming is subjective. Still i will rename it as MapStore to make it more consistent with SyncStore
|
I didnt add the label because i put the PR with 'WIP' tag. I will put the labels now. after doing the suggested changes |
Codecov Report
@@ Coverage Diff @@
## master #536 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 226 230 +4
Lines 4389 4460 +71
=====================================
+ Hits 4389 4460 +71
Continue to review full report at Codecov.
|
07_errors/mohitnag/error.go
Outdated
|
|
||
| const ( | ||
| // InvalidValue is used when the puppy value is below 0 | ||
| InvalidValue = "invalid input" |
There was a problem hiding this comment.
Invalid value sounds odd.
How about just invalid or invalidinput?
07_errors/mohitnag/mapstore.go
Outdated
| m[p.ID] = p | ||
| return nil | ||
| } | ||
| return ErrorF(Duplicate, "puppy with Id %d already exists", p.ID) |
There was a problem hiding this comment.
Imo invert logic: if ok... Then error.
It's more consistent with the ifs usually containing the error case and "bail early on error"
There was a problem hiding this comment.
Completely agree.
There was a problem hiding this comment.
Doesn't look like this has been applied yet. Can you please do so?
07_errors/mohitnag/types.go
Outdated
| } | ||
|
|
||
| // MapStore stores Puppy details with Puppy Id as Key and Puppy as value | ||
| type MapStore map[uint32]Puppy |
There was a problem hiding this comment.
Move to mapstore.go
Same below
There was a problem hiding this comment.
Looking really good @mohitnag. I have left a couple of comments throughout, the main one which I thing should be addressed is the format of the messages, I feel that the code should be an int code. But please let me know if you think otherwise.
07_errors/mohitnag/error.go
Outdated
| Duplicate = "puppy already exists" | ||
| ) | ||
|
|
||
| // Error is the authorisationservice package error with a code for comparison |
There was a problem hiding this comment.
I think this comment should be updated. Reference to the authorisationservice package is external to this project and should not be assumed knowledge. Just provide a description of what the struct is.
07_errors/mohitnag/error.go
Outdated
|
|
||
| // ErrorF is a utility function creating an error with given code and message | ||
| func ErrorF(code, format string, args ...interface{}) *Error { | ||
| message := fmt.Sprintf(format, args...) |
There was a problem hiding this comment.
No need to declare a separate message variable if we can return straight away.
return &Error{Code: code, Message: fmt.Sprintf(format, args...)}| err := ErrorF(NotFound, "test error message") | ||
| assert.Equal(NotFound, err.Code) | ||
| assert.Equal("test error message", err.Message) | ||
| assert.Equal("test error message :(code: puppy not found)", err.Error()) |
There was a problem hiding this comment.
I find this error message to be a bit misleading, and I think it comes from the fact that a string type is being used to store the code of the error. In my opinion I think a message format like below would be more appropriate:
assert.Equal("puppy not found (404)", err.Error())There was a problem hiding this comment.
I think error code in string type are more readable and easy to understand. However i do understand that int code like 404 is conventional. I would prefer keeping as it is, but let me know your thoughts and I will update accordingly.
07_errors/mohitnag/mapstore.go
Outdated
| m[p.ID] = p | ||
| return nil | ||
| } | ||
| return ErrorF(Duplicate, "puppy with Id %d already exists", p.ID) |
There was a problem hiding this comment.
Doesn't look like this has been applied yet. Can you please do so?
mohitnag
added
Under review
Ready for review
and removed
Changes requested
Under review
labels
| if _, ok := s.Load(p.ID); ok { | ||
| return ErrorF(Duplicate, "puppy with Id %d already exists", p.ID) | ||
| } | ||
| val, _ := strconv.Atoi(p.Value) |
There was a problem hiding this comment.
nit: might be dangerous to ignore the error here.
Fixes #535
Review of colleague's PR #534
Changes proposed in this PR: