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

Puppy Errors #485

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
169 changes: 169 additions & 0 deletions 07_errors/nicholascross/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
package main

import (
"fmt"
"io"
"os"
"sync"
)

type Puppy struct {
ID int64
Breed string
Colour string
Value float64
}

type Storer interface {
CreatePuppy(p Puppy) (bool, error)
RetrievePuppy(int64) (*Puppy, error)
UpdatePuppy(int64, Puppy) (bool, error)
DeletePuppy(int64) (bool, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having both a bool and error return value seems redundant. Whenever the bool is true, error is nil and vice versa. You can just return an error, and all is good if it is nil.

This applies to CreatePuppy, UpdatePuppy and DeletePuppy

}

type MapStore map[int64]Puppy

const (
missingPup = iota
Copy link
Contributor

Choose a reason for hiding this comment

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

Using Pup in these error codes and in the error type, but using Puppy everywhere is inconsistent. It would help if you used Puppy everywhere instead of Pup.

invalidPupValue
pupAlreadyExists
)

type PupError struct {
Message string
Code int
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 one error type with a code to differentiate the errors, Go tends towards defining different error types for different errors named FooError for parameterised errors, or package-level variables that are error values created with errors.New() named ErrFoo for static errors.

See https://github.com/golang/go/blob/master/src/encoding/json/encode.go#L230 for an example of the former and https://github.com/golang/go/blob/master/src/encoding/gob/decode.go#L19 for an example of the latter.

Or just create them on the fly as needed: https://github.com/golang/go/blob/master/src/database/sql/sql.go#L542

https://github.com/golang/go/wiki/Errors also has a bit more to say about this too.

I think the main thing is that your errors don't look like errors (from a Go perspective). return nil, missingPuppy(id) does not say err(or) anywhere which is what I would usually expect to see in Go code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so it looks like I didn't read the lab description where it says "Add a custom error type Error with fields Message and Code" - so ignore my comment about using Code (or not, as you please). I think my last paragraph in this comment still stands though - the errors don't look like errors, so if you can put "[Ee]rr" in the names somewhere I think it would read better.

}

func (e *PupError) Error() string {
return fmt.Sprintf("[%d] %s", e.Code, e.Message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting the code into the formatted string doesn't add any useful information because the code is meaningless to humans. The error string contains all the useful information. I'd suggest leaving it out.

}

func (e *PupError) IsMissingPup() bool {
return e.Code == missingPup
}

func (e *PupError) PupAlreadyExists() bool {
return e.Code == pupAlreadyExists
}

func (e *PupError) InvalidPupValue() bool {
return e.Code == invalidPupValue
}

func missingPuppy(id int64) error {
return &PupError{fmt.Sprintf("Puppy not found: %d", id), missingPup}
Copy link
Contributor

Choose a reason for hiding this comment

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

Error strings should not start with a capital, as they are often wrapped and this leads to capitals in the middle of the message. See https://github.com/golang/go/wiki/Errors for more details.

This applies to the other two errors too.

}

func puppyAlreadyExists(id int64) error {
return &PupError{fmt.Sprintf("Puppy already exists: %d", id), pupAlreadyExists}
}

func invalidPuppyValue(value float64) error {
return &PupError{fmt.Sprintf("Puppy value must be non negative: %f", value), invalidPupValue}
}

var out io.Writer = os.Stdout

func main() {
p := Puppy{1, "Jack Russel Terrier", "white and brown", 550.0}
fmt.Fprintf(out, "%d - %s [%s]: %f", p.ID, p.Breed, p.Colour, p.Value)
}

func (ms MapStore) CreatePuppy(p Puppy) (bool, error) {
if p.Value < 0 {
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 know about dogs, but I'm pretty sure my cat has negative value.

return false, invalidPuppyValue(p.Value)
}

nicholascross marked this conversation as resolved.
Show resolved Hide resolved
if _, ok := ms[p.ID]; ok {
return false, puppyAlreadyExists(p.ID)
}

ms[p.ID] = p
return true, nil
}

func (ms MapStore) RetrievePuppy(id int64) (*Puppy, error) {
if _, ok := ms[id]; ok {
pup := ms[id]
return &pup, nil
}
return nil, missingPuppy(id)
}

func (ms MapStore) UpdatePuppy(id int64, p Puppy) (bool, error) {
if p.Value < 0 {
return false, invalidPuppyValue(p.Value)
}

if _, ok := ms[id]; ok {
p.ID = id
ms[p.ID] = p
return true, nil
}
return false, missingPuppy(id)
}

func (ms MapStore) DeletePuppy(id int64) (bool, error) {
if _, ok := ms[id]; ok {
delete(ms, id)
return true, nil
}
return false, missingPuppy(id)
}

type SyncStore struct {
store *sync.Map
lock *sync.Mutex
}

func (s SyncStore) CreatePuppy(p Puppy) (bool, error) {
if p.Value < 0 {
return false, invalidPuppyValue(p.Value)
}

s.lock.Lock()
defer s.lock.Unlock()

if _, ok := s.store.Load(p.ID); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use LoadOrStore (https://golang.org/pkg/sync/#Map.LoadOrStore) without needing the mutex (and also simpler code):

if _, ok := s.LoadOrStore(p.ID, p); ok {
        return false, puppyAlreadyExists(p.ID)
}
return true, nil

Even if this is run concurrently with one of the methods that takes the mutex, they do not require the puppy not to exist between the two sync.Map operations so CreatePuppy done this way does not interfere.

Although as I think about it, perhaps keeping the mutex in this method is better as it is clearer. However, LoadOrStore is still a shorter way to implement this.

return false, puppyAlreadyExists(p.ID)
}

s.store.Store(p.ID, p)
return true, nil
}

func (s SyncStore) RetrievePuppy(id int64) (*Puppy, error) {
if pup, ok := s.store.Load(id); ok {
p := pup.(Puppy)
return &p, nil
}
return nil, missingPuppy(id)
}

func (s SyncStore) UpdatePuppy(id int64, p Puppy) (bool, error) {
if p.Value < 0 {
return false, invalidPuppyValue(p.Value)
}

s.lock.Lock()
defer s.lock.Unlock()

if _, ok := s.store.Load(id); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Align the happy path to the left edge (https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88) by inverting the sense of the conditional:

So,

if _, ok := s.store.Load(id); !ok {
        return false, missingPuppy(id)
}
p.ID = id
s.store.Store(p.ID, p)
return true, nil

p.ID = id
s.store.Store(p.ID, p)
return true, nil
}
return false, missingPuppy(id)
}

func (s SyncStore) DeletePuppy(id int64) (bool, error) {

s.lock.Lock()
defer s.lock.Unlock()

if _, ok := s.store.Load(id); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, align the happy path to the left:

if _, ok := s.store.Load(id); !ok {
        return false, missingPuppy(id)
}
s.store.Delete(id)
return true, nil

s.store.Delete(id)
return true, nil
}
return false, missingPuppy(id)
}
119 changes: 119 additions & 0 deletions 07_errors/nicholascross/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package main

import (
"bytes"
"strconv"
"sync"
"testing"

"github.com/stretchr/testify/suite"
)

type StorerTestSuite struct {
suite.Suite
storer Storer
}

func (suite *StorerTestSuite) SetupTest() {
ok, err := suite.storer.CreatePuppy(Puppy{1, "Jack Russel", "white", 500.0})
if !ok && !err.(*PupError).PupAlreadyExists() {
suite.Fail("Failed to setup test suite")
}
}

func (suite *StorerTestSuite) TestCreate() {
ok, _ := suite.storer.CreatePuppy(Puppy{12, "Komondor", "white", 1000.0})
puppy, _ := suite.storer.RetrievePuppy(12)
suite.Equal(int64(12), puppy.ID)
suite.Equal("Komondor", puppy.Breed)
suite.Equal("white", puppy.Colour)
suite.Equal(1000.00, puppy.Value)
suite.Equal(true, ok)
}

func (suite *StorerTestSuite) TestCreateInvalid() {
ok, err := suite.storer.CreatePuppy(Puppy{12, "Komondor", "white", -1000.0})
suite.Equal(false, ok)
suite.Equal("[1] Puppy value must be non negative: -1000.000000", err.Error())
suite.Equal(true, err.(*PupError).InvalidPupValue())
}

func (suite *StorerTestSuite) TestRead() {
puppy, _ := suite.storer.RetrievePuppy(1)
suite.Equal(int64(1), puppy.ID)
suite.Equal("Jack Russel", puppy.Breed)
suite.Equal("white", puppy.Colour)
suite.Equal(500.00, puppy.Value)
}

func (suite *StorerTestSuite) TestUpdate() {
ok, _ := suite.storer.UpdatePuppy(1, Puppy{1, "Jack Russel Terrier", "white and brown", 550.0})
puppy, _ := suite.storer.RetrievePuppy(1)
suite.Equal(int64(1), puppy.ID)
suite.Equal("Jack Russel Terrier", puppy.Breed)
suite.Equal("white and brown", puppy.Colour)
suite.Equal(550.00, puppy.Value)
suite.Equal(true, ok)
}

func (suite *StorerTestSuite) TestUpdateMissing() {
ok, err := suite.storer.UpdatePuppy(10, Puppy{10, "Jack Russel Terrier", "white and brown", 550.0})
suite.Equal(false, ok)
suite.Equal("[0] Puppy not found: 10", err.Error())
suite.Equal(true, err.(*PupError).IsMissingPup())
}

func (suite *StorerTestSuite) TestUpdateInvalid() {
ok, err := suite.storer.UpdatePuppy(1, Puppy{1, "Jack Russel Terrier", "white and brown", -550.0})
suite.Equal(false, ok)
suite.Equal("[1] Puppy value must be non negative: -550.000000", err.Error())
suite.Equal(true, err.(*PupError).InvalidPupValue())
}

func (suite *StorerTestSuite) TestDelete() {
ok, _ := suite.storer.DeletePuppy(1)
puppy, _ := suite.storer.RetrievePuppy(1)
suite.Nil(puppy)
suite.Equal(true, ok)
}

func (suite *StorerTestSuite) TestDeleteMissing() {
ok, err := suite.storer.DeletePuppy(10)
suite.Equal(false, ok)
suite.Equal("[0] Puppy not found: 10", err.Error())
suite.Equal(true, err.(*PupError).IsMissingPup())
}

func TestMapStorer(t *testing.T) {
s := StorerTestSuite{
storer: MapStore{},
}
suite.Run(t, &s)
}

func TestSyncStorer(t *testing.T) {
store := sync.Map{}
lock := sync.Mutex{}

s := StorerTestSuite{
storer: SyncStore{
store: &store,
lock: &lock,
},
}
suite.Run(t, &s)
}

func TestMainOutput(t *testing.T) {
var buf bytes.Buffer
out = &buf

main()

expected := strconv.Quote("1 - Jack Russel Terrier [white and brown]: 550.000000")
actual := strconv.Quote(buf.String())

if expected != actual {
t.Errorf("Unexpected output. Expected: %q - Actual: %q", expected, actual)
}
}