Skip to content

第5回課題(大倉) #39

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ookura-mf
Copy link

No description provided.

Copy link
Owner

@mf-sakura mf-sakura left a comment

Choose a reason for hiding this comment

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

@ookura-mf
概ね良さそうに思いました:+1:
エラーハンドリング周りで1点だけコメントしました。

Comment on lines +73 to +74
err := userController.Update(*id, *firstName, *lastName)
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

Goだと以下の様にする事が多いです。
1行になってシンプルになったり、変数のスコープがif文に狭まるのでメモリ効率が良いです。

Suggested change
err := userController.Update(*id, *firstName, *lastName)
if err != nil {
if err := userController.Update(*id, *firstName, *lastName); err != nil {

Copy link
Author

Choose a reason for hiding this comment

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

なるほど 👀
rubyだとifで代入するのは可読性が悪いので設定によってはlinterに怒られたりしますが、ここではerrに代入するだけなのでこの方が良さそうですね

Comment on lines +42 to +43
_, err := tx.Exec("UPDATE users SET first_name = ?, last_name = ? WHERE id = ?", u.FirstName, u.LastName, u.ID)
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

これも一つ目の戻り値を使わないのであれば、以下の方が良いと思います。

Suggested change
_, err := tx.Exec("UPDATE users SET first_name = ?, last_name = ? WHERE id = ?", u.FirstName, u.LastName, u.ID)
if err != nil {
if _, err := tx.Exec("UPDATE users SET first_name = ?, last_name = ? WHERE id = ?", u.FirstName, u.LastName, u.ID); err !=nil{}

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

Successfully merging this pull request may close these issues.

2 participants