-
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
Implement custom error handling to Storer interface #590
Changes from 2 commits
1358c73
bb43faf
c52f22a
5fa4311
8efff56
e6dbc8d
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,32 @@ | ||
package main | ||
|
||
import "fmt" | ||
|
||
type ErrorCode int | ||
|
||
type Error struct { | ||
Code ErrorCode | ||
Message string | ||
} | ||
|
||
const ( | ||
NegativeValue ErrorCode = iota + 1001 | ||
IDNotFound | ||
Unknown = 1999 | ||
) | ||
|
||
func (e *Error) Error() string { | ||
return fmt.Sprintf("[%d] %s", e.Code, e.Message) | ||
} | ||
|
||
// NewError creates a new error with the given enum | ||
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. nitpick: Godoc comments should end with a full-stop, as they appear as sentences in the documentation (https://blog.golang.org/godoc-documenting-go-code). another nitpick: I'd say "given error code" instead of "given enum" - seems a little clearer to me. |
||
func NewError(code ErrorCode) error { | ||
switch code { | ||
case NegativeValue: | ||
return &Error{code, "Puppy value must be greater than 0"} | ||
case IDNotFound: | ||
return &Error{code, "Nonexistent Puppy ID"} | ||
default: | ||
return &Error{Unknown, "Unknown 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. This throws away the error code passed in, which may be useful when trying to debug where a strange error is coming from. Perhaps set the string as |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
package main | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestUnknownError(t *testing.T) { | ||
a := assert.New(t) | ||
|
||
error := NewError(123) | ||
a.Equal(error, NewError(Unknown)) | ||
} | ||
|
||
func TestError(t *testing.T) { | ||
a := assert.New(t) | ||
|
||
error := Error{1, "message"} | ||
formattedError := error.Error() | ||
a.Equal(formattedError, "[1] 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. I don't see that you have 100% test coverage with this commit. There are a couple of branches in I will share the magic in a subsequent comment where I have noticed your tests failing... |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package main | ||
|
||
func main() { | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package main | ||
|
||
import ( | ||
"bytes" | ||
"io" | ||
"os" | ||
"testing" | ||
) | ||
|
||
var out io.Writer = os.Stdout | ||
|
||
func TestMainOutput(t *testing.T) { | ||
var buf bytes.Buffer | ||
out = &buf | ||
|
||
main() | ||
|
||
expected := "" | ||
|
||
got := buf.String() | ||
|
||
if expected != got { | ||
t.Errorf("\nExpected: %s\nGot: %s", expected, got) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package main | ||
|
||
// NewMapStore returns a pointer to a new instance of the MapStore struct which implements the Storer interface. | ||
func NewMapStore() Storer { | ||
return &MapStore{ | ||
uuid: 0, | ||
store: map[int]Puppy{}, | ||
} | ||
} | ||
|
||
// Create increments the uuid and adds the provided Puppy struct to the store with this identifier. | ||
func (store *MapStore) Create(puppy Puppy) int { | ||
puppy.ID = store.uuid | ||
store.store[puppy.ID] = puppy | ||
store.uuid++ | ||
|
||
return puppy.ID | ||
} | ||
|
||
// Read returns the puppy matching the provided uuid. | ||
// An empty Puppy struct is returned if the identifier does not exist. | ||
func (store *MapStore) Read(id int) Puppy { | ||
if _, ok := store.store[id]; ok { | ||
return store.store[id] | ||
} | ||
|
||
return Puppy{} | ||
} | ||
|
||
// Update modifies the puppy matching the provided uuid in the store with the provided Puppy struct. | ||
// It returns a bool whether a matching identifier was modified or not. | ||
func (store *MapStore) Update(id int, puppy Puppy) bool { | ||
if _, ok := store.store[id]; !ok { | ||
return false | ||
} | ||
|
||
puppy.ID = id | ||
store.store[id] = puppy | ||
return true | ||
} | ||
|
||
// Destroy removes the puppy matching the provided uuid from the store. | ||
// It returns a bool whether a matching identifier was deleted or not. | ||
func (store *MapStore) Destroy(id int) bool { | ||
if _, ok := store.store[id]; !ok { | ||
return false | ||
} | ||
|
||
delete(store.store, id) | ||
return true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
package main | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/suite" | ||
) | ||
|
||
type StoreSuite struct { | ||
suite.Suite | ||
store Storer | ||
} | ||
|
||
func (suite *StoreSuite) TestCreate() { | ||
a := assert.New(suite.T()) | ||
id := suite.store.Create(Puppy{Breed: "Wolf", Color: "Grey", Value: "450"}) | ||
|
||
a.Equal(id, 0) | ||
} | ||
|
||
func (suite *StoreSuite) TestCreateSecond() { | ||
a := assert.New(suite.T()) | ||
id := suite.store.Create(Puppy{Breed: "Boxer", Color: "Brown", Value: "300"}) | ||
|
||
a.Equal(id, 1) | ||
} | ||
|
||
func (suite *StoreSuite) TestRead() { | ||
a := assert.New(suite.T()) | ||
data := suite.store.Read(0) | ||
|
||
a.Equal(data, Puppy{ID: 0, Breed: "Wolf", Color: "Grey", Value: "450"}) | ||
} | ||
|
||
func (suite *StoreSuite) TestReadNonExistent() { | ||
a := assert.New(suite.T()) | ||
success := suite.store.Read(100) | ||
|
||
a.Equal(success, Puppy{}) | ||
} | ||
|
||
func (suite *StoreSuite) TestUpdate() { | ||
a := assert.New(suite.T()) | ||
success := suite.store.Update(0, Puppy{Breed: "Doberman", Color: "Black", Value: "500"}) | ||
data := suite.store.Read(0) | ||
|
||
a.Equal(success, true) | ||
a.Equal(data, Puppy{ID: 0, Breed: "Doberman", Color: "Black", Value: "500"}) | ||
} | ||
|
||
func (suite *StoreSuite) TestUpdateNonExistent() { | ||
a := assert.New(suite.T()) | ||
success := suite.store.Update(100, Puppy{Breed: "Doberman", Color: "Black", Value: "500"}) | ||
|
||
a.Equal(success, false) | ||
} | ||
|
||
func (suite *StoreSuite) TestDestroy() { | ||
a := assert.New(suite.T()) | ||
success := suite.store.Destroy(1) | ||
data := suite.store.Read(1) | ||
|
||
a.Equal(success, true) | ||
a.Equal(data, Puppy{}) | ||
} | ||
|
||
func (suite *StoreSuite) TestDestroyNonExistent() { | ||
a := assert.New(suite.T()) | ||
success := suite.store.Destroy(100) | ||
|
||
a.Equal(success, false) | ||
} | ||
|
||
func (suite *StoreSuite) TestIdIncrementOnDelete() { | ||
a := assert.New(suite.T()) | ||
id := suite.store.Create(Puppy{Breed: "Greyhound", Color: "Light Brown", Value: "700"}) | ||
suite.store.Destroy(id) | ||
newID := suite.store.Create(Puppy{Breed: "Greyhound", Color: "Light Brown", Value: "700"}) | ||
|
||
a.Equal(newID, 3) | ||
} | ||
|
||
func TestStore(t *testing.T) { | ||
suite.Run(t, &StoreSuite{store: NewMapStore()}) | ||
suite.Run(t, &StoreSuite{store: NewSyncStore()}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package main | ||
|
||
// NewSyncStore returns a pointer to a new instance of the SyncStore struct which implements the Storer interface. | ||
func NewSyncStore() Storer { | ||
return &SyncStore{ | ||
uuid: 0, | ||
} | ||
} | ||
|
||
// Create increments the uuid and adds the provided Puppy struct to the store with this identifier. | ||
func (store *SyncStore) Create(puppy Puppy) int { | ||
puppy.ID = store.uuid | ||
store.Store(puppy.ID, puppy) | ||
store.uuid++ | ||
|
||
return puppy.ID | ||
} | ||
|
||
// Read returns the puppy matching the provided uuid. | ||
// An empty Puppy struct is returned if the identifier does not exist. | ||
func (store *SyncStore) Read(id int) Puppy { | ||
if value, ok := store.Load(id); ok { | ||
return value.(Puppy) | ||
} | ||
|
||
return Puppy{} | ||
} | ||
|
||
// Update modifies the puppy matching the provided uuid in the store with the provided Puppy struct. | ||
// It returns a bool whether a matching identifier was modified or not. | ||
func (store *SyncStore) Update(id int, puppy Puppy) bool { | ||
if _, ok := store.Load(id); !ok { | ||
return false | ||
} | ||
|
||
puppy.ID = id | ||
store.Store(id, puppy) | ||
return true | ||
} | ||
|
||
// Destroy removes the puppy matching the provided uuid from the store. | ||
// It returns a bool whether a matching identifier was deleted or not. | ||
func (store *SyncStore) Destroy(id int) bool { | ||
if _, ok := store.Load(id); !ok { | ||
return false | ||
} | ||
|
||
store.Delete(id) | ||
return true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package main | ||
|
||
import ( | ||
"sync" | ||
) | ||
|
||
type Puppy struct { | ||
ID int | ||
Breed string | ||
Color string | ||
Value string | ||
} | ||
|
||
type Storer interface { | ||
Create(puppy Puppy) int | ||
Read(ID int) Puppy | ||
Update(ID int, puppy Puppy) bool | ||
Destroy(ID int) bool | ||
} | ||
|
||
type MapStore struct { | ||
uuid int | ||
store map[int]Puppy | ||
} | ||
|
||
type SyncStore struct { | ||
uuid int | ||
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.
It is not clear why you have decided to start at 1001 for the error codes, and also set
Unknown
to 1999. What if in future you need >1000 error codes? Since this is an enum, the actual values for the enum are not relevant, unless they map to some external system. In that case you should document that mapping. But if there is one, don't suggest there is one but you're not telling by using a different set of meaningless numbers to the default.I'd suggest starting with "Unknown = iota" and going up from there. That does not give you that artificial limit of the number of error codes when you do eventually get to more than 1000 error codes (which will come up in lab 22).
Perhaps you have chosen to use 1000+ so you get a consistent formatting width when converting the error to a string, but I'd suggest you do formatting at the place where formatting is specified (i.e.
[%04d]
)