Alain #8
Alain #8
Conversation
* Added return and borrow endpoints * Added custom errors for return and borrow
wildeng
requested review from
gr-eg,
DhruvishaSupeda,
JamesBalazs,
carlotaarvela and
tomw-1
| libraryController := new(controllers.LibraryController) | ||
|
|
||
| router := martini.Classic() | ||
| router.Map(ctx) |
There was a problem hiding this comment.
Nice, i didn't think of handing the ctx/client to martini to allow DI in the endpoints
There was a problem hiding this comment.
yes, I've seen that with martini you can map whatever and I said why not? In this case is more Rails :D
There was a problem hiding this comment.
Quite Spring as well (@Autowired and @service or @component)
| github.com/go-martini/martini v0.0.0-20170121215854-22fa46961aab | ||
| github.com/joho/godotenv v1.4.0 | ||
| google.golang.org/api v0.60.0 | ||
| ) | ||
|
|
||
| require ( |
There was a problem hiding this comment.
This is quite the require block. Did the IDE pull these in for you? I'd be interested to know what libs it thought it needed all these for
There was a problem hiding this comment.
I'm using neovim as IDE and I have this plugin set up:
" Go development
Plug 'fatih/vim-go', { 'do': ':GoUpdateBinaries' }
There was a problem hiding this comment.
something we need to keep an eye on then. It maybe that these are indirect dependencies that end up in the go.sum file when we build it (but GoLand handles it differently to neovim)
There was a problem hiding this comment.
I will keep an eye on it and maybe is just a question of config
| @@ -0,0 +1,35 @@ | |||
| package helpers | |||
|
|
|||
| type BoorrowError struct { | |||
| return true | ||
| } | ||
|
|
||
| type IBorrowError interface { |
There was a problem hiding this comment.
Personally, I'm not a big fan of the I notation to show an interface (and I don't think its a pattern you'll see in the pendo-appengine codebase)
There was a problem hiding this comment.
in this case what is the common usage?
There was a problem hiding this comment.
BorrowErrorInterface and then BorrowErrorImplementation for the concrete version. Naming conventions is on my list of things to talk through with the core team soon and document so we can have a consistent standard
| } | ||
|
|
||
| type IBorrowError interface { | ||
| error |
There was a problem hiding this comment.
Nice use of the anonymous field in the struct
| func createQuery(filters url.Values) *datastore.Query { | ||
| if len(filters) > 0 { | ||
| if author := filters.Get("author"); author != "" { | ||
| return datastore.NewQuery("Book").Filter("Author=", author).Order("Id") |
There was a problem hiding this comment.
How would this change if you were to allow people to filter by author AND title?
There was a problem hiding this comment.
from the docs should be
datastore.NewQuery("Book").Filter("Author=", author).Filter("Title=", title).Order("Id")| @@ -1,8 +1,97 @@ | |||
| package models | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
Wider point for discussion, but I wouldn't have these methods in the model book object. I would have them in a book service, with the model simply there to define the struct (maybe a Ruby thing as these methods would sit in the ActiveRecord model class?)
There was a problem hiding this comment.
Wider point for discussion, but I wouldn't have these methods in the model book object. I would have them in a book service, with the model simply there to define the struct (maybe a Ruby thing as these methods would sit in the ActiveRecord model class?)
yes, it's a wider point of discussion this.
I can say that it's not a Ruby thing because the bare minimum ActiveRecord file is just the class definition itself and honestly I also had this kind of discussion while I was using Java. What's the right place for the business logic? Not in the controller imho, but where between the model and a service class? Here I applied what I've been using more in Java and was the model. Not sure which one is best practice in Go, but it would be nice to learn
There was a problem hiding this comment.
Yeah, interesting because if were doing this in Java, i would have had a data access layer that would do the get/list etc. So it would be controller -> service -> data access. So the service is business logic, the data access handles the actual getting/updating. With the model on the side being referenced by all the above. Something to nail down in the coming weeks
There was a problem hiding this comment.
This is where a common style guide would be useful
| if err != nil { | ||
| return book, err | ||
| } | ||
| if book.Borrowed == true { |
There was a problem hiding this comment.
if book.Borrowed == true is the same as if book.Borrowed
| if err != nil { | ||
| return book, err | ||
| } | ||
| if book.Borrowed == false { |
There was a problem hiding this comment.
as above by if !book.Borrowed
|
|
||
| book, err := models.BorrowBook(client, ctx, id) | ||
|
|
||
| if err, isBorrow := err.(helpers.IBorrowError); isBorrow && err.IsBorrowed() { |
There was a problem hiding this comment.
I can see what you are trying to do here. but as you are already throwing a quite specific error, with a appropriate error message, i don't think this block is very useful and can be covered by the generic err != nil below
There was a problem hiding this comment.
This is my bad, I wanted to have a specific error for BadRequest and then do
if err != nil {
writeError(err, http. StatusInternalServerError, w)
return
}but I didn't change the latter
My first try at it :)