Skip to content
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

Kadai4 misonog #57

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

Kadai4 misonog #57

wants to merge 5 commits into from

Conversation

misonog
Copy link

@misonog misonog commented May 9, 2021

おみくじ API

お手数ですがレビューお願いいたします。

仕様

  • JSON 形式でおみくじの結果を返す
  • 正月(1/1-1/3)だけ大吉にする
  • ハンドラのテストを書いてみる

利用方法

setup

$ make  # テスト & ビルド

サーバーの起動

$ ./omikuji-server

おみくじを引く

$ curl "http://127.0.0.1:8080/"
> {"result":"小吉"}
$ curl "http://127.0.0.1:8080/?date=2021-01-01"
> {"result":"大吉"}

ディレクトリ構造

.
├── Makefile
├── README.md
├── go.mod
├── main.go
├── omikuji-server
├── omikuji.go
└── omikuji_test.go

Copy link

@sryoya sryoya left a comment

Choose a reason for hiding this comment

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

いくつかコメントしてみました


func (o *omikuji) Draw() {
if isShogatsu(o.date) {
o.result = daikichi
Copy link

Choose a reason for hiding this comment

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

ここでreturnしてしまえば、elseはいらないですね

if isShogatsu(o.date) {
o.result = daikichi
} else {
rand.Seed(time.Now().UnixNano())
Copy link

Choose a reason for hiding this comment

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

rand.Seedみたいな共通の初期化系の処理は、init functionで最初に実行させてしまうという方法があります

https://golang.org/doc/effective_go#init

if dateValue != "" {
date, err := time.Parse(timeForm, dateValue)
if err != nil {
log.Fatal(err)
Copy link

Choose a reason for hiding this comment

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

この場合、httpのerrorとして返した方が良いと思います。特に、ユーザーが異常な値を入れたら発生する想定内なエラーだと思うので

handler(w, r)
rw := w.Result()
defer rw.Body.Close()
if rw.StatusCode != http.StatusOK {
Copy link

Choose a reason for hiding this comment

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

StatusCodeも期待値に入れると、異常系のテストもできます Bad Requestのときとかも含め

Comment on lines +22 to +25
type omikuji struct {
result string
date time.Time
}
Copy link

Choose a reason for hiding this comment

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

この2つのデータをstructとしてまとめて置く必要はありますか?

Comment on lines +13 to +16
daikichi = "大吉"
)

var omikujiResults = []string{"吉", "小吉", "凶"}
Copy link

Choose a reason for hiding this comment

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

daikichi以外の値も定数に定義して、daikichiも含めてこのsliceに入れてしまう方が可変性、可読性(一覧性)ともに高いと思います。

}

_, month, date := t.Date()
if month == time.January {
Copy link

Choose a reason for hiding this comment

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

ここがfalseの時点で return false すれば、ifをネストしなくてよくなりますね

)

func main() {
http.HandleFunc("/", handler)
Copy link

Choose a reason for hiding this comment

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

これをmain関数の外に置いておくことでhttpのroutingも含めてテストできるようになります

@misonog
Copy link
Author

misonog commented Jun 22, 2021

@sryoya レビューありがとうございます!別途確認いたします。

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