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.

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
32 changes: 32 additions & 0 deletions 07_errors/patrickmarabeas/errors.go
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
Copy link
Contributor

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])

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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"}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 fmt.Sprintf("Unknown error (%d)", code) if code != Unknown? Just an idea.

}
}
22 changes: 22 additions & 0 deletions 07_errors/patrickmarabeas/errors_test.go
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")
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 NewError() that are not tested. They probably get tested in a later commit by using it, but since you have structured your PR as a series of small cohesive commits (good), you should ensure each commit passes all the tests and has complete coverage. There is some git rebase --exec magic you can use to run all your tests and coverage checks on every one of your commits automagically, but that usually requires a makefile or script to run all the checks you want to.

I will share the magic in a subsequent comment where I have noticed your tests failing...

5 changes: 5 additions & 0 deletions 07_errors/patrickmarabeas/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package main

func main() {

}
25 changes: 25 additions & 0 deletions 07_errors/patrickmarabeas/main_test.go
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)
}
}
51 changes: 51 additions & 0 deletions 07_errors/patrickmarabeas/mapStore.go
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
}
87 changes: 87 additions & 0 deletions 07_errors/patrickmarabeas/store_test.go
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()})
}
50 changes: 50 additions & 0 deletions 07_errors/patrickmarabeas/syncStore.go
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
}
29 changes: 29 additions & 0 deletions 07_errors/patrickmarabeas/types.go
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
}