internal/api: New function improvements
Closed this issue · 0 comments
zalgonoise commented
The New function is too convoluted for what it is doing, and could be greatly simplified.
- The function signature is scoping to a type parameter (of type Service, an interface). Although generics are great, this particular type parameter is not improving the code's readability, nor does it make any difference if the function were to simply accept an interface as input. Type parameters are extra useful when working with concrete types, and only slightly useful when working with complex generic objects (structs and interfaces alike). Proposing changing the signature to:
New(port int, svc Service) API
- According to Gin's documentation, best practices would be to create the engine from a designated function for it, like
gin.New()
orgin.Default()
, instead ofnew(gin.Engine)
. The variable is immediately shadowed right after by a (correct) call togin.New()
- The configuration for
r.Use(gin.Recovery())
should be documented in particular for added clarity. I understand now that it will recover from panics and return 500-status-code HTTP responses, but I had to browse the methods documentation to know that. - The return statement of this function is a method call of a struct built in-place. This is not exactly friendly to the eye, and even less if the type happened to be a pointer. While the reason for the method call is covered below, these two steps should be done separately simply for readability.
The API type exposes two private methods, withServer
and withRoutes
; both of which return an API (itself). These methods imply these are optional procedures when configuring the server, like one would order a hotdog with cheese or with fries. However, the actual purpose of these methods is to configure the HTTP server in two distinct manners, both required regardless: configuring the HTTP server, and configuring the routes in the multiplexer. Note the issues in these methods:
- the API type is not a pointer. This means that the methods mutating these fields are not actually performing any lasting changes unless the returned object is used. The answer is not to make it a pointer either, but to include its actions in the New function instead.
- The
withRoutes
method is never called, so the multiplexer is never configured in the first place - The
withServer
method is overly complex. It creates a channel of API and configures the HTTP server in a go routine, to then return the same object (the receiver!) to the channel. Only thea.srv
definition matters, here.