Conversation
| return nil, err | ||
| } | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
This error handler doesn't seem to be checking anything new.
| } | ||
|
|
||
| // LoraWAN used to communicate to LoRaWAN external system | ||
| type LoraWAN struct { |
There was a problem hiding this comment.
Could this client package/struct encapsulate more information about the API endpoints and their request/response?
For example, rather than having the LoraWan.DoPost method, what if we had something that looked more like:
func (l *LoraWAN) OnboardSensor(req OnboardSensorRequest) (*OnboardSensorResponse, error)This would mean that the specifics of marshalling/unmarshalling the request/response would be contained within this package and wouldn't be leaking out into the processor package. If this endpoint were to change slightly, an engineer would have to make changes in both this package and in the processor package.
|
|
||
| // DoPost sends data via POST (HTTP) request | ||
| func (l *LoraWAN) DoPost(body io.Reader, ctx context.Context) (resp *http.Response, err error) { | ||
| fullUrl := l.baseURL + endpoint |
There was a problem hiding this comment.
It might be safer to use something like url.JoinPath.
| } | ||
| } | ||
|
|
||
| func registerDevice(loraWAN client.LoraWAN, ctx context.Context) (*device.Device, error) { |
There was a problem hiding this comment.
It's more usual in Go for context.Context to be the first argument - most Go engineers would find this arrangement very unnatural.
| } | ||
| } | ||
|
|
||
| func registerDevice(loraWAN client.LoraWAN, ctx context.Context) (*device.Device, error) { |
There was a problem hiding this comment.
Using an interface here for the client is a really nice choice and enables easier testing, however, it often yields better results to define interfaces where they are used, not where they are implemented.
Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values. The implementing package should return concrete (usually pointer or struct) types: that way, new methods can be added to implementations without requiring extensive refactoring.
https://github.com/golang/go/wiki/CodeReviewComments#interfaces
| if err := godotenv.Load(".env"); err != nil { | ||
| panic("error loading.env file") | ||
| } | ||
| godotenv.Load(".env") |
There was a problem hiding this comment.
This line seems to do the same as the above line.
| godotenv.Load(".env") | ||
| baseurl := os.Getenv("BASE_URL") | ||
|
|
||
| maxConcurrentJobs, err := strconv.Atoi(os.Getenv("MAX_CONCURRENT_JOBS")) |
There was a problem hiding this comment.
This set of env var loading/parsing gets in the way of reading what main() is doing, it could make sense to extract these out into another function that returns a struct containing all the configuration values.
| codeProcessor := &processor.CodeProcessor{ | ||
| CodeRegistrationLimit: 1, | ||
| MaxConcurrentJobs: maxConcurrentJobs, | ||
| LoraWAN: *loraWAN, |
There was a problem hiding this comment.
Dependency injection of the client here is nice 👍
| } | ||
|
|
||
| hasChar := false | ||
| for _, char := range allowedChars { |
There was a problem hiding this comment.
This set of assertions for each character is really verbose and repetitive, could you loop over each character in the string instead ?
| }(count) | ||
|
|
||
| // Spawn workers | ||
| for job := 0; job < codeRegistrationLimit; job++ { |
There was a problem hiding this comment.
It would be neater if CodeProcessor had a .Run() method that span up the worker GoRoutines. This method could accept a context.Context to handle shutdown, or CodeProcessor could have a method called Stop().
2 participants
No description provided.