Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement custom error handling to Storer interface #590

Closed
wants to merge 6 commits into from
Closed

Implement custom error handling to Storer interface #590

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1358c73
Copy from upstream/06_puppy/patrickmarabeas
patrickmarabeas Aug 19, 2019
bb43faf
Add custom error handling
patrickmarabeas Aug 19, 2019
c52f22a
Extends Storer interface for all methods to also return an error
patrickmarabeas Aug 19, 2019
5fa4311
Implement custom error handling
patrickmarabeas Aug 19, 2019
8efff56
Add locking to use of sync.Map
patrickmarabeas Aug 19, 2019
e6dbc8d
Add dummy output to main
patrickmarabeas Aug 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions 07_errors/patrickmarabeas/mapStore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,43 +9,50 @@ func NewMapStore() Storer {
}

// Create increments the uuid and adds the provided Puppy struct to the store with this identifier.
func (store *MapStore) Create(puppy Puppy) int {
func (store *MapStore) Create(puppy Puppy) (int, error) {
if puppy.Value < 0 {
return -1, NewError(NegativeValue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be return 0, NewError(NegativeValue)

When an error is returned, the value of the other return parameters should be the zero value for their type. Otherwise people may start checking the wrong value for errors, or try to use those values thinking there might be some significance to the non-zero-value.

}

puppy.ID = store.uuid
store.store[puppy.ID] = puppy
store.uuid++

return puppy.ID
return puppy.ID, nil
}

// 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 {
func (store *MapStore) Read(id int) (Puppy, error) {
if _, ok := store.store[id]; ok {
return store.store[id]
return store.store[id], nil
}

return Puppy{}
return Puppy{}, NewError(IDNotFound)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments about this function (and I know my comments really apply to lab 6, not this one but here we are now):

  1. Usually you want to align the happy path to the left edge, so the indented case should be returning the error,
  2. Since you have the non-error path inside the if, rather than indexing the map again you should save the value rather than using the blank identifier:
if p, ok := store.store[id]; ok {
    return p, nil
}

unless you use a var to declare p outside the scope of the if. Combined, it would look like:

func (store *MapStore) Read(id int) (Puppy, error) {
    var p Puppy
    if p, ok := store.store[id]; !ok {
        return Puppy{}, NewError(IDNotFound)
    }
    return p, nil
}

}

// 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 {
func (store *MapStore) Update(id int, puppy Puppy) (bool, error) {
if _, ok := store.store[id]; !ok {
return false
return false, NewError(IDNotFound)
}
if puppy.Value < 0 {
return false, NewError(NegativeValue)
}

puppy.ID = id
store.store[id] = puppy
return true
return true, nil
}

// 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 {
func (store *MapStore) Destroy(id int) (bool, error) {
if _, ok := store.store[id]; !ok {
return false
return false, NewError(IDNotFound)
}

delete(store.store, id)
return true
return true, nil
}
59 changes: 44 additions & 15 deletions 07_errors/patrickmarabeas/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,70 +14,99 @@ type StoreSuite struct {

func (suite *StoreSuite) TestCreate() {
a := assert.New(suite.T())
id := suite.store.Create(Puppy{Breed: "Wolf", Color: "Grey", Value: "450"})

id, error := suite.store.Create(Puppy{Breed: "Wolf", Color: "Grey", Value: 450})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be called err not error as error is the built-in error type.

a.Equal(id, 0)
a.Equal(error, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a.NoError(err) rather than comparing to nil

}

func (suite *StoreSuite) TestCreateSecond() {
a := assert.New(suite.T())
id := suite.store.Create(Puppy{Breed: "Boxer", Color: "Brown", Value: "300"})

id, error := suite.store.Create(Puppy{Breed: "Boxer", Color: "Brown", Value: 300})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/error/err/

a.Equal(id, 1)
a.Equal(error, nil)
}

func (suite *StoreSuite) TestCreateNegativeNumber() {
a := assert.New(suite.T())

id, error := suite.store.Create(Puppy{Breed: "Wolf", Color: "Grey", Value: -100})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/error/err/

a.Equal(id, -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id should be irrelevant for the error case - no point testing it.

a.Equal(error, NewError(NegativeValue))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a.Error(...)

}

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"})
data, error := suite.store.Read(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/error/err/

Going to stop saying this now. Please fix throughout

a.Equal(data, Puppy{ID: 0, Breed: "Wolf", Color: "Grey", Value: 450})
a.Equal(error, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a.NoError(err)

Going to stop saying this now. Please fix throughout.

}

func (suite *StoreSuite) TestReadNonExistent() {
a := assert.New(suite.T())
success := suite.store.Read(100)

success, error := suite.store.Read(100)
a.Equal(success, Puppy{})
a.Equal(error, NewError(IDNotFound))
}

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)

success, error := suite.store.Update(0, Puppy{Breed: "Doberman", Color: "Black", Value: 500})
a.Equal(success, true)
a.Equal(data, Puppy{ID: 0, Breed: "Doberman", Color: "Black", Value: "500"})
a.Equal(error, nil)

data, error := suite.store.Read(0)
a.Equal(data, Puppy{ID: 0, Breed: "Doberman", Color: "Black", Value: 500})
a.Equal(error, nil)
}

func (suite *StoreSuite) TestUpdateNonExistent() {
a := assert.New(suite.T())
success := suite.store.Update(100, Puppy{Breed: "Doberman", Color: "Black", Value: "500"})

success, error := suite.store.Update(100, Puppy{Breed: "Doberman", Color: "Black", Value: 500})
a.Equal(success, false)
a.Equal(error, NewError(IDNotFound))
}

func (suite *StoreSuite) TestUpdateNegativeNumber() {
a := assert.New(suite.T())

success, error := suite.store.Update(0, Puppy{Breed: "Doberman", Color: "Black", Value: -500})
a.Equal(success, false)
a.Equal(error, NewError(NegativeValue))
}

func (suite *StoreSuite) TestDestroy() {
a := assert.New(suite.T())
success := suite.store.Destroy(1)
data := suite.store.Read(1)

success, error := suite.store.Destroy(1)
a.Equal(success, true)
a.Equal(error, nil)

data, error := suite.store.Read(1)
a.Equal(data, Puppy{})
a.Equal(error, NewError(IDNotFound))
}

func (suite *StoreSuite) TestDestroyNonExistent() {
a := assert.New(suite.T())
success := suite.store.Destroy(100)

success, error := suite.store.Destroy(100)
a.Equal(success, false)
a.Equal(error, NewError(IDNotFound))
}

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"})
id, _ := suite.store.Create(Puppy{Breed: "Greyhound", Color: "Light Brown", Value: 700})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than ignoring the error here, perhaps use require?

import "github.com/stretchr/testify/require"
...
require.NoError(suite.T(), err)

require will stop the test there, and I find it useful to use it to differentiate between test setup that is failing and the results of the tests themselves (for which I use assert).

success, _ := suite.store.Destroy(id)
a.Equal(success, true)

newID, _ := suite.store.Create(Puppy{Breed: "Greyhound", Color: "Light Brown", Value: 700})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar. You do not expect this Create() call to fail, so require that it does not.

a.Equal(newID, 3)
}

Expand Down
29 changes: 18 additions & 11 deletions 07_errors/patrickmarabeas/syncStore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,43 +8,50 @@ func NewSyncStore() Storer {
}

// Create increments the uuid and adds the provided Puppy struct to the store with this identifier.
func (store *SyncStore) Create(puppy Puppy) int {
func (store *SyncStore) Create(puppy Puppy) (int, error) {
if puppy.Value < 0 {
return -1, NewError(NegativeValue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to mapstore - use a zero value for unspecified return values.

}

puppy.ID = store.uuid
store.Store(puppy.ID, puppy)
store.uuid++

return puppy.ID
return puppy.ID, nil
}

// 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 {
func (store *SyncStore) Read(id int) (Puppy, error) {
if value, ok := store.Load(id); ok {
return value.(Puppy)
return value.(Puppy), nil
}

return Puppy{}
return Puppy{}, NewError(IDNotFound)
}

// 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 {
func (store *SyncStore) Update(id int, puppy Puppy) (bool, error) {
if _, ok := store.Load(id); !ok {
return false
return false, NewError(IDNotFound)
}
if puppy.Value < 0 {
return false, NewError(NegativeValue)
}

puppy.ID = id
store.Store(id, puppy)
return true
return true, nil
}

// 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 {
func (store *SyncStore) Destroy(id int) (bool, error) {
if _, ok := store.Load(id); !ok {
return false
return false, NewError(IDNotFound)
}

store.Delete(id)
return true
return true, nil
}