-
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
Puppy Errors #485
Puppy Errors #485
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,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) | ||
} | ||
|
||
type MapStore map[int64]Puppy | ||
|
||
const ( | ||
missingPup = iota | ||
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. 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 | ||
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. Rather than one error type with a code to differentiate the errors, Go tends towards defining different error types for different errors named 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). 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. Ok, so it looks like I didn't read the lab description where it says "Add a custom error type |
||
} | ||
|
||
func (e *PupError) Error() string { | ||
return fmt.Sprintf("[%d] %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. 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} | ||
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. 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 { | ||
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 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 { | ||
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 should be able to use
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 Although as I think about it, perhaps keeping the mutex in this method is better as it is clearer. However, |
||
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 { | ||
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. 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,
|
||
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 { | ||
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. Similarly, align the happy path to the left:
|
||
s.store.Delete(id) | ||
return true, nil | ||
} | ||
return false, missingPuppy(id) | ||
} |
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) | ||
} | ||
} |
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.
Having both a
bool
anderror
return value seems redundant. Whenever thebool
istrue
,error
isnil
and vice versa. You can just return an error, and all is good if it isnil
.This applies to
CreatePuppy
,UpdatePuppy
andDeletePuppy