(WIP)Implement Lab 07 : Custom Errors #664
Conversation
sirigithub
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe,
rokane and
samuong
as code owners
Codecov Report
@@ Coverage Diff @@
## master #664 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 234 238 +4
Lines 4613 4681 +68
=====================================
+ Hits 4613 4681 +68
Continue to review full report at Codecov.
|
sirigithub
changed the title
| return nil | ||
| } | ||
|
|
||
| func dbConn(dbfilePath string) (db *leveldb.DB) { |
There was a problem hiding this comment.
dbConn - dbfilePath is unused (from unparam)
| ErrIDNotFound = 404 | ||
| ErrMarshallData = 400 | ||
| ErrUrmarshallData = 400 | ||
| ErrDatabaseConn = 500 |
There was a problem hiding this comment.
ErrDatabaseConn is unused (from deadcode)
| ErrUrmarshallData = 400 | ||
| ErrDatabaseConn = 500 | ||
| ErrDatabaseWrite = 500 | ||
| ErrDatabaseRead = 500 |
There was a problem hiding this comment.
ErrDatabaseRead is unused (from deadcode)
| currID int | ||
| } | ||
|
|
||
| func NewdbStore(filePath string) *dbStore { |
There was a problem hiding this comment.
exported func NewdbStore returns unexported type *main.dbStore, which can be annoying to use (from golint)
| func NewdbStore(filePath string) *dbStore { | ||
| var store = dbStore{} | ||
| store.dbfilePath = filePath | ||
| return &store |
There was a problem hiding this comment.
You can implement this function with a single statement:
return &dbStore{dbfilePath: filePath}
However, it would probably be better to open the database when the dbStore is created, and add a Close() error method so the caller can close it later (which implements the io.Closer interface:
type dbStore struct {
conn *db.leveldb.DB
currID int
}
func NewdbStore(filePath string) *dbStore {
return &dbStore{conn: dbConn(filePath)} // one day I'll handle errors here :-)
}
func (s *dbStore) Close() error {
return s.conn.Close()
}
But you'll also need a different approach for currID. Because the database is persistent, if the process restarts, you start allocating IDs from 1 again. These will conflict with existing IDs.
A different approach is to use github.com/google/uuid to generate random IDs. That way do you not need to maintain any state to allocate IDs.
| value := getPuppyData(puppy) | ||
| err := db.Put([]byte(strconv.Itoa(puppy.ID)), value, nil) | ||
| if err != nil { | ||
| return -1, NewError(ErrDatabaseWrite, fmt.Sprintf("Error creating Puppy %d in the dbStore", puppy.ID)) |
There was a problem hiding this comment.
Generally you always want to wrap errors so you don't lose any detail so I'd write NewError() to take third error parameter (which can be nil) and store that error in your error type. See more detail below.
| type Error struct { | ||
| Message string | ||
| Code int | ||
| } |
There was a problem hiding this comment.
Since you will often wrap other errors, I would include an err error field here too so you can store the error returned from leveldb.
| ) | ||
|
|
||
| func (e *Error) Error() string { | ||
| return fmt.Sprintf("Error code %d: %s", e.Code, e.Message) |
There was a problem hiding this comment.
with the err field, this would be fmt.Sprintf("error code %d: %s: %v", e.Code, e.Message, e.err)
Note that I've made the first letter lower case as per https://github.com/golang/go/wiki/CodeReviewComments#error-strings
| value := getPuppyData(puppy) | ||
| err := db.Put([]byte(strconv.Itoa(puppy.ID)), value, nil) | ||
| if err != nil { | ||
| return -1, NewError(ErrDatabaseWrite, fmt.Sprintf("Error creating Puppy %d in the dbStore", puppy.ID)) |
There was a problem hiding this comment.
Don't use capital letters to start error strings: https://github.com/golang/go/wiki/CodeReviewComments#error-strings
|
The go-course is now closed. Thank you very much for participating. |
Fixes #665
Review of colleague's PR TBD
Changes proposed in this PR: