Implement custom error handling to Storer interface #590

patrickmarabeas · 2019-08-19T08:44:35Z

Implements custom error handling to the Storer interface. All methods must now return an error.

Errors that are covered:

Puppy value < 0
Puppy ID not found

Fixes / improvements also included:

The value property in Puppy has had its type changed to an int to represent a numerical cent figure so that it may be properly throw an error if the value supplied is less than 0
Proper locking in SyncMap

Fixes #589

Adds a NewError function which creates a new error of the custom error type `Error` (contraining fields: `Message` and `Code`) with the given enum. Error enums for the package begin at 1001. Errors being handled: - Puppy value < 0 - Puppy ID not found Lab 07 (anz-bank#589)

In order to implement the Storer interface, all methods must now return and error. The value property in Puppy has had its type changed to an int to represent a numerical cent figure so that it may be properly throw an error if the value supplied is less than 0. Lab 07 (anz-bank#589)

- Value < 0 in Create and Update - ID not found in Read, Update and Delete Lab 07 (anz-bank#589)

07_errors/patrickmarabeas/main.go

Empty main has a code coverage of 0%, unlike lab-06.

codecov · 2019-08-19T09:27:16Z

Codecov Report

Merging #590 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #590   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         172    176    +4     
  Lines        3197   3270   +73     
=====================================
+ Hits         3197   3270   +73

Impacted Files	Coverage Δ
07_errors/patrickmarabeas/mapStore.go	`100% <100%> (ø)`
07_errors/patrickmarabeas/errors.go	`100% <100%> (ø)`
07_errors/patrickmarabeas/main.go	`100% <100%> (ø)`
07_errors/patrickmarabeas/syncStore.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0f8dbd...e6dbc8d. Read the comment docs.

camscale

Generally pretty good. And I really like the split up of commits, but there's one issue there with making all commits buildable and working.

main() is a bit odd - maybe it makes sense to use the storers you implement somehow, but i note there is no requirement to in the README, so i guess that's ok!

Have a read of your commit messages - i noticed a spelling mistake in one but github does not allow me to comment at the commit level, and now I've forgotten exactly which one it was.

camscale · 2019-08-26T01:52:09Z

07_errors/patrickmarabeas/errors.go

+}
+
+const (
+	NegativeValue ErrorCode = iota + 1001


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

camscale · 2019-08-26T01:53:30Z

07_errors/patrickmarabeas/errors.go

+	return fmt.Sprintf("[%d] %s", e.Code, e.Message)
+}
+
+// NewError creates a new error with the given enum


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.

camscale · 2019-08-26T01:55:23Z

07_errors/patrickmarabeas/errors.go

+	case IDNotFound:
+		return &Error{code, "Nonexistent Puppy ID"}
+	default:
+		return &Error{Unknown, "Unknown error"}


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.

camscale · 2019-08-26T02:08:16Z

07_errors/patrickmarabeas/errors_test.go

+	error := Error{1, "message"}
+	formattedError := error.Error()
+	a.Equal(formattedError, "[1] message")
+}


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...

camscale · 2019-08-26T02:09:45Z

07_errors/patrickmarabeas/types.go


 type Puppy struct {
 	ID    int
 	Breed string
 	Color string
-	Value string
+	Value int // cents


Two ways you could convey this information without a comment:

Change the field name to ValueCents

Add a new type: type Cents int

Just a thought.

camscale · 2019-08-26T02:40:53Z

07_errors/patrickmarabeas/store_test.go


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


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

camscale · 2019-08-26T02:42:19Z

07_errors/patrickmarabeas/syncStore.go

-func (store *SyncStore) Create(puppy Puppy) int {
+func (store *SyncStore) Create(puppy Puppy) (int, error) {
+	if puppy.Value < 0 {
+		return -1, NewError(NegativeValue)


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

camscale · 2019-08-26T02:43:23Z

07_errors/patrickmarabeas/syncStore.go

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


The usual Go pattern is to put this right after the Lock() with defer:

store.Lock() defer store.Unlock()

This way if you later add an early return in the middle for error handling, you do not forget to unlock.

camscale · 2019-08-26T02:44:12Z

07_errors/patrickmarabeas/syncStore.go

 	if value, ok := store.Load(id); ok {
 		return value.(Puppy), nil
 	}
+	store.RUnlock()


This is a bug, because you are not unlocking the store inside the if statement. Instead use the defer pattern above and not worry about it any more.

camscale · 2019-08-26T02:44:37Z

07_errors/patrickmarabeas/syncStore.go

 	store.Delete(id)
+	store.Unlock()


ditto. use the defer pattern, even for a simple one line block like this one.

juliaogris · 2020-02-08T03:02:19Z

The go-course is now closed. Thank you very much for participating.

patrickmarabeas added 4 commits August 19, 2019 18:26

Copy from upstream/06_puppy/patrickmarabeas

1358c73

Lab 07 (anz-bank#589)

Implement custom error handling

5fa4311

- Value < 0 in Create and Update - ID not found in Read, Update and Delete Lab 07 (anz-bank#589)

patrickmarabeas added Ready for review lab7 labels Aug 19, 2019

patrickmarabeas requested review from anzboi, anzdaddy, camscale, joshcarp, juliaogris, linuxmonk, pentaphobe, rokane and samuong as code owners August 19, 2019 08:44

Add locking to use of sync.Map

8efff56

patrickmarabeas force-pushed the lab-07-pm branch from aa191bc to add520a Compare August 19, 2019 09:15

golangcibot reviewed Aug 19, 2019

View reviewed changes

07_errors/patrickmarabeas/main.go Outdated Show resolved Hide resolved

Add dummy output to main

e6dbc8d

Empty main has a code coverage of 0%, unlike lab-06.

patrickmarabeas force-pushed the lab-07-pm branch from add520a to e6dbc8d Compare August 19, 2019 09:21

juliaogris added the Under review label Aug 26, 2019

camscale suggested changes Aug 26, 2019

View reviewed changes

camscale added Changes requested and removed Ready for review Under review labels Aug 26, 2019

juliaogris closed this Feb 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement custom error handling to Storer interface #590

Implement custom error handling to Storer interface #590

patrickmarabeas commented Aug 19, 2019

codecov bot commented Aug 19, 2019

camscale left a comment

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

camscale Aug 26, 2019

juliaogris commented Feb 8, 2020


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

Implement custom error handling to Storer interface #590

Implement custom error handling to Storer interface #590

Conversation

patrickmarabeas commented Aug 19, 2019

codecov bot commented Aug 19, 2019

Codecov Report

camscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaogris commented Feb 8, 2020