New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
實作 #53 Get popular board list #80
實作 #53 Get popular board list #80
Conversation
boards, err := delivery.usecase.GetPopularBoards(context.Background()) | ||
if err != nil { | ||
// TODO: record error | ||
delivery.logger.Warningf("find popular board failed: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
因為使用者沒辦法順利進行某一項功能了,也許這邊應該是使用 Errorf 會更好?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯的確,我馬上改
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if err != nil { | ||
// TODO: record error | ||
delivery.logger.Errorf("find popular board failed: %v", err) | ||
w.WriteHeader(http.StatusInternalServerError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建議把w.WriteHeader 放在w.Write上一行, 閱讀起來的連貫性比較好
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好的
t.Logf("got response %v", rr.Body.String()) | ||
responseData := responseMap["data"] | ||
popularBoards := responseData.(map[string]interface{})["items"].([]interface{}) | ||
for i := range popularBoards[1:] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可以寫得更直觀一點,另外atoi失敗或不按順序排列就直接t.Fatalf,不用再比較下去了
for i := range popularBoards[1:] { | |
var prevNum int | |
for i := range popularBoards{ | |
curr := popularBoards[i].(map[string]interface{})["number_of_user"] | |
currNum, err := strconv.Atoi(curr.(string)) | |
if err != nil { | |
t.Fatalf("handler returned unexpected body, invalid number_of_user: got %v", | |
currNum) | |
} | |
if i > 0 && prevNum < currNum { | |
t.Fatalf("handler returned unexpected body, invalid order: got %v before %v", | |
prevNum, currNum) | |
} | |
prevNum = currNum | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯了解,感謝!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
📝 相關的 issue / Related Issues
⛏ 變更內容 / Details of Changes
因為之前再寫的時候 ptt-backend 是蠻舊版本的,所以就重新 merge、增加了簡單的測試並重發 pr。