-
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
Create error type for handling erros #607
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package main | ||
|
||
import "fmt" | ||
|
||
type Error struct { | ||
Message string | ||
Code ErrorCode | ||
} | ||
|
||
func (e *Error) Error() string { | ||
return fmt.Sprintf("Error code:%d,Error:%s", e.Code, e.Message) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the additional new line |
||
} | ||
|
||
// Errorf creates a new Error with formatting | ||
func Errorf(code ErrorCode, format string, args ...interface{}) *Error { | ||
return ErrorEf(code, format, args...) | ||
} | ||
hsy3418 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// ErrorEf creates a new Error with causing error and formatting | ||
func ErrorEf(code ErrorCode, format string, args ...interface{}) *Error { | ||
return &Error{ | ||
Message: fmt.Sprintf(format, args...), | ||
Code: code, | ||
} | ||
} | ||
|
||
//ErrCode defines the kind of error | ||
type ErrorCode uint8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice work 👍 |
||
|
||
//Error codes | ||
const ( | ||
// ErrInvalidInput for invalid user input | ||
ErrInvalidInput ErrorCode = iota | ||
// ErrDuplicate is used when attempting to create an already existing entry | ||
ErrDuplicate | ||
// ErrNotFound is used when trying to access a non-existing entry | ||
ErrNotFound | ||
// ErrNotFound is used when initialisation or setup fails | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package main | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestInvalidInputError(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could refactor these into a table style test approach. All three of these have the same structure, where the only difference is the error code, error message, and output. Make these as variables inside your cases and you can repeat the logic. |
||
err := Errorf(ErrInvalidInput, "invalid input") | ||
assert.Equal(t, err.Code, ErrInvalidInput) | ||
errMessage := err.Error() | ||
assert.Equal(t, "Error code:0,Error:invalid input", errMessage) | ||
} | ||
|
||
func TestNotFoundError(t *testing.T) { | ||
err := Errorf(ErrNotFound, "error not found") | ||
assert.Equal(t, err.Code, ErrNotFound) | ||
errMessage := err.Error() | ||
assert.Equal(t, "Error code:2,Error:error not found", errMessage) | ||
} | ||
|
||
func TestDuplicateError(t *testing.T) { | ||
err := Errorf(ErrDuplicate, "duplicate value") | ||
assert.Equal(t, err.Code, ErrDuplicate) | ||
errMessage := err.Error() | ||
assert.Equal(t, "Error code:1,Error:duplicate value", errMessage) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package main | ||
|
||
import ( | ||
"fmt" | ||
"io" | ||
"os" | ||
) | ||
|
||
var out io.Writer = os.Stdout | ||
|
||
func main() { | ||
p1 := Puppy{ID: 101, Breed: "Poodle", Colour: "White", Value: 1280.5} | ||
p2 := Puppy{ID: 102, Breed: "Poodle", Colour: "Grey", Value: 1340.5} | ||
|
||
mapStore := NewMapStore() | ||
syncStore := NewSyncStore() | ||
|
||
_ = mapStore.CreatePuppy(p1) | ||
_ = syncStore.CreatePuppy(p2) | ||
|
||
puppy, _ := mapStore.ReadPuppy(101) | ||
puppy2, _ := syncStore.ReadPuppy(102) | ||
fmt.Fprintf(out, "Puppy ID %d is %v", puppy.ID, puppy.Value) | ||
fmt.Fprintf(out, "Puppy ID %d is %v", puppy2.ID, puppy2.Value) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the additional new line |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package main | ||
|
||
import ( | ||
"bytes" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestMain(t *testing.T) { | ||
assert := assert.New(t) | ||
var buf bytes.Buffer | ||
out = &buf | ||
main() | ||
expected := "Puppy ID 101 is 1280.5Puppy ID 102 is 1340.5" | ||
actual := buf.String() | ||
assert.Equal(expected, actual) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package main | ||
|
||
// MapStore is a implementation of storer for the storage of puppies | ||
type MapStore struct { | ||
puppies map[int32]Puppy | ||
} | ||
|
||
func NewMapStore() *MapStore { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please document |
||
return &MapStore{ | ||
puppies: map[int32]Puppy{}, | ||
} | ||
} | ||
|
||
// CreatePuppy adds a nuw puppy to the puppies store | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adds a new puppy to the puppy store |
||
func (m *MapStore) CreatePuppy(puppy Puppy) error { | ||
if puppy.Value < 0 { | ||
return ErrorEf(ErrInvalidInput, "The puppy value is invalidate") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "puppy value is invalid" |
||
} | ||
if _, exists := m.puppies[puppy.ID]; !exists { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple of things here, I mentioned in the previous pr that normal convention for golang is if error -> fail early, and that I was a little more relaxed on it when there is only a single error constraint. Now that this has multiple I think this should be flipped so that the final return is the puppy and the error duplicate being returned if the puppy exists. Given that, I also think that this should not be the case where the ID is passed in the puppy and validated on creation. I think an ID should be assigned and returned back to the user in the response. This was raised in your previous PR, so when you update it there can you please bring it through to this one as well. |
||
m.puppies[puppy.ID] = puppy | ||
return nil | ||
} | ||
return ErrorEf(ErrDuplicate, "This puppy exists ") | ||
} | ||
|
||
// ReadPuppy retrieves the puppy for a given id from puppies store | ||
func (m *MapStore) ReadPuppy(id int32) (Puppy, error) { | ||
if _, exists := m.puppies[id]; !exists { | ||
return Puppy{}, ErrorEf(ErrNotFound, "This puppy does not exist") | ||
} | ||
return m.puppies[id], nil | ||
} | ||
|
||
//UpdatePuppy updates the puppy for the given id | ||
func (m *MapStore) UpdatePuppy(puppy Puppy) error { | ||
if puppy.Value < 0 { | ||
return ErrorEf(ErrInvalidInput, "The puppy value is invalidate") | ||
} | ||
if _, exists := m.puppies[puppy.ID]; !exists { | ||
return ErrorEf(ErrNotFound, "This puppy does not exist") | ||
} | ||
m.puppies[puppy.ID] = puppy | ||
return nil | ||
} | ||
|
||
//DeletePuppy delete the puppy for the given id from puppies store | ||
func (m *MapStore) DeletePuppy(id int32) error { | ||
if _, exists := m.puppies[id]; exists { | ||
delete(m.puppies, id) | ||
return nil | ||
} | ||
return ErrorEf(ErrNotFound, "This puppy does not exist") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
package main | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/suite" | ||
) | ||
|
||
func (suite *storerTestSuite) SetupTest() { | ||
suite.store = suite.makeStore() | ||
suite.toBeCreatedPuppy = Puppy{ID: 101, Breed: "Poodle", Colour: "White", Value: 1000.5} | ||
suite.existsPuppy = Puppy{ID: 102, Breed: "Poodle", Colour: "White", Value: 1280.5} | ||
suite.toBeUpdatedPuppy = Puppy{ID: 102, Breed: "Poodle", Colour: "White", Value: 2000} | ||
suite.invalidPuppy = Puppy{ID: 103, Breed: "Poodle", Colour: "White", Value: -1000} | ||
suite.invaildError = ErrorEf(ErrInvalidInput, "The puppy value is invalidate") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then this can be simplified by removing these lines |
||
suite.invalidIDError = ErrorEf(ErrNotFound, "This puppy does not exist") | ||
suite.dupicatesError = ErrorEf(ErrDuplicate, "This puppy exists ") | ||
err := suite.store.CreatePuppy(suite.existsPuppy) | ||
if err != nil { | ||
suite.FailNow("Failed to setup test") | ||
} | ||
} | ||
|
||
type storerTestSuite struct { | ||
suite.Suite | ||
store Storer | ||
makeStore func() Storer | ||
toBeCreatedPuppy Puppy | ||
existsPuppy Puppy | ||
toBeUpdatedPuppy Puppy | ||
invalidPuppy Puppy | ||
invaildError error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These three errors are constant and only used for comparative assertions. You should make these constants, having global scope and then you won't have to have the assignment code within the const (
ErrorInvalidInput = ErrorEf(ErrInvalidInput, "The puppy value is invalidate")
ErrorNotFound = ErrorEf(ErrNotFound, "This puppy does not exist")
ErrorDuplicate = ErrorEf(ErrDuplicate, "This puppy exists ")
) |
||
invalidIDError error | ||
dupicatesError error | ||
} | ||
|
||
//successfully create puppy, new puppy, existing puppy, value <0 | ||
func (suite *storerTestSuite) TestCreatePuppy() { | ||
assert := assert.New(suite.T()) | ||
testCases := []struct { | ||
title string | ||
input Puppy | ||
expected error | ||
}{ | ||
{"Create new puppy", suite.toBeCreatedPuppy, nil}, | ||
{"Create existing puppy", suite.toBeCreatedPuppy, suite.dupicatesError}, | ||
{"Create a invalid puppy", suite.invalidPuppy, suite.invaildError}, | ||
} | ||
for _, tc := range testCases { | ||
tc := tc | ||
suite.T().Run(tc.title, func(t *testing.T) { | ||
err := suite.store.CreatePuppy(tc.input) | ||
assert.Equal(tc.expected, err) | ||
}) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary blank line. |
||
} | ||
|
||
// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like you have gone to put a comment here and then forgotten. Test's don't really need documentation on their function names, so this can be removed. |
||
func (suite *storerTestSuite) TestUpdatePuppy() { | ||
assert := assert.New(suite.T()) | ||
testCases := []struct { | ||
title string | ||
inputPuppy Puppy | ||
expectedError error | ||
}{ | ||
{"Update puppy successfully", suite.toBeUpdatedPuppy, nil}, | ||
{"Update a invalid puppy", suite.invalidPuppy, suite.invaildError}, | ||
{"Update non-existing puppy", Puppy{}, suite.invalidIDError}, | ||
} | ||
for _, tc := range testCases { | ||
tc := tc | ||
suite.T().Run(tc.title, func(t *testing.T) { | ||
err := suite.store.UpdatePuppy(tc.inputPuppy) | ||
assert.Equal(tc.expectedError, err) | ||
}) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another extra new line not required |
||
} | ||
|
||
func (suite *storerTestSuite) TestReadPuppy() { | ||
assert := assert.New(suite.T()) | ||
testCases := []struct { | ||
title string | ||
input int32 | ||
expected Puppy | ||
expectedError error | ||
}{ | ||
{"Read puppy successfully", suite.existsPuppy.ID, suite.existsPuppy, nil}, | ||
{"Read non-existing puppy", suite.toBeCreatedPuppy.ID, Puppy{}, suite.invalidIDError}, | ||
} | ||
for _, tc := range testCases { | ||
tc := tc | ||
suite.T().Run(tc.title, func(t *testing.T) { | ||
readPuppy, err := suite.store.ReadPuppy(tc.input) | ||
assert.Equal(tc.expected, readPuppy) | ||
assert.Equal(tc.expectedError, err) | ||
}) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again here |
||
} | ||
|
||
func (suite *storerTestSuite) TestDeletePuppy() { | ||
assert := assert.New(suite.T()) | ||
testCases := []struct { | ||
title string | ||
input int32 | ||
expectedError error | ||
}{ | ||
{"Delete puppy successfully", suite.existsPuppy.ID, nil}, | ||
{"Delete non-existing puppy", suite.toBeCreatedPuppy.ID, suite.invalidIDError}, | ||
} | ||
for _, tc := range testCases { | ||
tc := tc | ||
suite.T().Run(tc.title, func(t *testing.T) { | ||
err := suite.store.DeletePuppy(tc.input) | ||
assert.Equal(tc.expectedError, err) | ||
}) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here |
||
} | ||
|
||
func TestStorers(t *testing.T) { | ||
suite.Run(t, &storerTestSuite{ | ||
makeStore: func() Storer { return NewMapStore() }, | ||
}) | ||
suite.Run(t, &storerTestSuite{ | ||
makeStore: func() Storer { return NewSyncStore() }, | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package main | ||
|
||
import ( | ||
"sync" | ||
) | ||
|
||
type SyncStore struct { | ||
sync.Mutex | ||
sync.Map | ||
} | ||
|
||
func NewSyncStore() *SyncStore { | ||
return &SyncStore{} | ||
} | ||
|
||
// CreatePuppy adds a nuw puppy to the puppies store | ||
func (m *SyncStore) CreatePuppy(puppy Puppy) error { | ||
m.Lock() | ||
defer m.Unlock() | ||
if puppy.Value < 0 { | ||
return ErrorEf(ErrInvalidInput, "The puppy value is invalidate") | ||
} | ||
if _, exists := m.Load(puppy.ID); exists { | ||
return ErrorEf(ErrDuplicate, "This puppy exists ") | ||
} | ||
m.Store(puppy.ID, puppy) | ||
return nil | ||
} | ||
|
||
// ReadPuppy retrieves the puppy for a given id from puppies store | ||
func (m *SyncStore) ReadPuppy(id int32) (Puppy, error) { | ||
if p, exists := m.Load(id); exists { | ||
puppy, _ := p.(Puppy) | ||
return puppy, nil | ||
} | ||
return Puppy{}, ErrorEf(ErrNotFound, "This puppy does not exist") | ||
|
||
} | ||
|
||
//UpdatePuppy updates the puppy for the given id | ||
func (m *SyncStore) UpdatePuppy(puppy Puppy) error { | ||
m.Lock() | ||
defer m.Unlock() | ||
if puppy.Value < 0 { | ||
return ErrorEf(ErrInvalidInput, "The puppy value is invalidate") | ||
} | ||
if _, exists := m.Load(puppy.ID); !exists { | ||
return ErrorEf(ErrNotFound, "This puppy does not exist") | ||
} | ||
m.Store(puppy.ID, puppy) | ||
return nil | ||
} | ||
|
||
//DeletePuppy delete the puppy for the given id from puppies store | ||
func (m *SyncStore) DeletePuppy(id int32) error { | ||
m.Lock() | ||
defer m.Unlock() | ||
if _, exists := m.Load(id); exists { | ||
m.Delete(id) | ||
return nil | ||
} | ||
return ErrorEf(ErrNotFound, "This puppy does not exist") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package main | ||
|
||
// Puppy defines the data structure corresponding to a pet | ||
type Puppy struct { | ||
ID int32 `json:"id"` | ||
Value float32 `json:"value"` | ||
Breed string `json:"breed"` | ||
Colour string `json:"colour"` | ||
} | ||
|
||
//Storer define standard CRUD operations for puppys | ||
type Storer interface { | ||
CreatePuppy(Puppy) error | ||
ReadPuppy(ID int32) (Puppy, error) | ||
UpdatePuppy(puppy Puppy) error | ||
DeletePuppy(ID int32) 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.
Since this struct and the below
Error()
method are also a part of the exported package can you please add documentation to these as well?