Skip to content

Conversation

Mirza-Samad-Ahmed-Baig
Copy link

  • Replace panic() calls with proper error logging in main.go
  • Fix potential nil pointer dereference when checking response.StatusCode()
  • Replace panic() calls with error returns in httper.go
  • Improve error handling for network operations and HTTP responses

- Replace panic() calls with proper error logging in main.go
- Fix potential nil pointer dereference when checking response.StatusCode()
- Replace panic() calls with error returns in httper.go
- Improve error handling for network operations and HTTP responses
Copy link

Copy link

@shedokan shedokan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took the liberty to share some Go knowledge.
Regarding errors - can read about them here - https://preslav.me/2023/04/14/golang-error-handling-is-a-form-of-storytelling/

}
if err != nil {
panic(err)
return ""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this "proper error handling"

Error should be bubbled up, so that whomever did the Post can handle it.

Empty content might be ok in some cases

Comment on lines +159 to +160
logger.Error("Failed to create route", zap.Error(err), zap.String("path", apiPath))
continue

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why continue loading the server? This hides server loading issues, and it takes a while for whomever ran this service to know something is off (as the process didn't exit)

Comment on lines +228 to +229
logger.Error("Server failed to start", zap.Error(err))
return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return is redundant

Suggested change
logger.Error("Server failed to start", zap.Error(err))
return
logger.Error("Server failed to start", zap.Error(err))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants