Kadai3 tomoyuki.kobayashi #36

bakisunsan · 2018-11-22T00:54:44Z

課題3

タイピングゲームを作ろう

標準出力に英単語を出す（出すものは自由）
標準入力から1行受け取る
制限時間内に何問解けたか表示する

下記のような構成で実現した

main.go
オプションパラメタの読み込み、設定ファイルの読み込み、
gameからチャネルに送られてくる情報を整形して出力する責務
/typing
- quiz_data.go
  語彙を格納し、ゲームレベルに応じた語彙を返却する責務
- question.go
  語彙をレベル毎に、ゲーム性のある(ランダムな)値を返却する責務
- game.go
  ゲーム全体の制御と、進行状況のデータ保持を行う。主にやっていることは下記
  - 進行状況からゲームの現在レベルを判定し、レベルにあった語彙をquestionチャネルに送信する
  - answerチャネルから入力を受けつけ、現在questionで出題されているものとマッチしたかミスしたかを判定して、成績を更新
  - 指定した時間でゲームを終了し、resultチャネルに成績データを送る

分割ダウンロードを行う

Rangeアクセスを用いる
いくつかのゴルーチンでダウンロードしてマージする
エラー処理を工夫する
golang.org/x/sync/errgourpパッケージなどを使ってみる
キャンセルが発生した場合の実装を行う

下記のような流れで実現した

HEADしてRangeサポートしているか/Contentsのサイズを見る
分割ダウンロードした一時ファイルを作成するディレクトリを作成
任意のプロセス数で、goroutineでRangeダウンロードを行い、一時ファイルに保存する
すでに対象ファイルが存在していたら、DLせずスキップする
errgroup.Groupで待ち合わせる
全ての一時ファイルを読んで結合してダウンロードファイルを作成
一時ファイルを削除

knsh14 · 2018-11-22T09:39:15Z

kadai3-1/src/tomoyukikobayashi/typing/questions.go

+}
+
+// Yaml HACK 使用しているパッケージの使用でしょうがなくpublicにしている
+type Yaml struct {


QuestionContainer とかそういう感じの名前にすると良さげですね
この struct は YAML データをパースした結果を入れるものですが、保存している先は別に YAML じゃなくても問題ありません。例えば JSON とかでも構わないわけです。
この struct の本質はレベルごとの問題を格納しているものなので、それに沿った名前にするほうが可読性も上がると思います。

ありがとうございます。おっしゃる通りで、こいつが何のためにいるのか、がわかる名前じゃないとダメですね
そもそもQuestionerの所でこの構造を知っている必要もないので、名前を変えてインターフェイスとかで渡すようにしようかと

…nsan/dojo4 into kadai3-tomoyuki.kobayashi

bakisunsan · 2018-11-25T14:49:28Z

kadai3-1/src/tomoyukikobayashi/typing/game.go

+}
+
+// Run ゲームを開始する
+func (c *constGame) Run() (<-chan string, <-chan string, <-chan [2]int) {


// TODO [2]int に名前をつけてやったほうがよかった

mizkei

コメントしましたので、参考にしていただければと思います

mizkei · 2018-11-26T03:31:32Z

kadai3-1/src/tomoyukikobayashi/typing/quiz_data.go

+
+// NewQuizData クイズデータを生成する
+func NewQuizData(source io.Reader) (QuizData, error) {
+	data, err := ioutil.ReadAll(source)


ioutil.ReadAllはメモリにすべて載せてしまうので、streamから読み出すようにすることでメモリを圧迫しなくなります
(今回の場合は大量のファイルを読み出したりしないため、あまり影響はないと思いますが)

var s QuizSource if err := yaml.NewDecoder(r).Decode(&s); err != nil { return nil, err } return &s, nil

あー、Reader(ストリーム) -> buf(メモリ) -> struct だとメモリ余計に確保するから
Reader(ストリーム) -> struct にしましょうという事ですね。確かに

mizkei · 2018-11-26T03:42:07Z

kadai3-1/src/tomoyukikobayashi/typing/questions.go

+}
+
+func (q *questionContainer) GetNewWord(level int) string {
+	rand.Seed(time.Now().UnixNano())


rand.Seed は毎回セットする必要はないため、一度だけ呼んであげればよいです

mizkei · 2018-11-26T03:55:57Z

kadai3-1/src/tomoyukikobayashi/typing/game.go

+}
+
+// NewGame Gameのコンストラクタです
+func NewGame(timeout int, source io.Reader, input io.Reader) (Game, error) {


同じ型の引数は型を省略できます

func NewGame(timeout int, source, input io.Reader) (Game, error) {...

確かに。そちらの方が可読性高いですね

mizkei · 2018-11-26T04:33:28Z

kadai3-1/src/tomoyukikobayashi/typing/game.go

+// Run ゲームを開始する
+func (c *constGame) Run() (<-chan string, <-chan string, <-chan [2]int) {
+	tc := time.Duration(c.timeout) * time.Second
+	// NOTE defer cancel() cancelするとDone条件閉じてゲーム終わっちゃう


https://golang.org/pkg/context/

Failing to call the CancelFunc leaks the child and its children until the parent is canceled or the timer fires.

大抵の場合、cancelはちゃんと呼んでほうが良いです

その上でここでcancelを呼べないのは、設計としてここで作成したcontextを扱うべきでないか、 Run がブロックしないからだと思います

Run(ctx context.Context) でキャンセル可能にすれば、ここでの作成は必要なくなり、Runを呼び出す側で管理できるようになると思います
ただ、その場合は ゲーム が扱うべき時間制限が外の扱いになってしまうのが気持ち悪いかもしれませんが、そもそもRunという長時間実行される可能性のあるメソッドにはcontext.Contextを渡して、キャンセル可能にしておくのは悪くないと思います

もう一つ、 Run をブロックさせるという方法ですが、このメソッドの中でcontextを作成した場合、このメソッドがreturnするときにはその処理は終わっていてほしいはずです(もし終わっていてほしくない場合はもはや別のcontextとして扱われるべき状況である可能性が高いはず)
したがって、Runはゲームが終了するまでブロックする形で実行されれば、ゲームの終了とcancelは同一のタイミングでよくなるため、このメソッドの中で defer cancel() が実行できるようになるはずです

CancelFunc cancels the child and its children, removes the parent's reference to the child

あー、Cancel呼んでやらないと、子コンテキストへの参照が生き続けるのか、それはマズイですね。。。

Run をブロックさせるという方法ですが、このメソッドの中でcontextを作成した場合、このメソッドがreturnするときにはその処理は終わっていてほしいはずです(もし終わっていてほしくない場合はもはや別のcontextとして扱われるべき状況である可能性が高いはず)

言われてみると、確かにそうですね
Contextを持っているRunは終わったけどそのContextを参照したgoroutineは元気に生きているというのはおかしい。。。

mizkei · 2018-11-26T04:35:35Z

kadai3-1/src/tomoyukikobayashi/typing/game.go

+	go func() {
+		for {
+			word := c.GetNewWord(c.nextLevel())
+			qCh <- word


ここでブロックしてしまうと、問題がchanから読み込まれるまでcontextによるキャンセルが効かなくなるので、selectに含めるとより扱いやすくなると思います

そうですね。selectにこれを追加して、defaultをなくすのが良さそうです
ありがとうございます。

mizkei · 2018-11-26T04:59:53Z

kadai3-1/src/tomoyukikobayashi/typing/game.go

+		for {
+			word := c.GetNewWord(c.nextLevel())
+			qCh <- word
+			c.currentWord = word


この c.currentWord の扱いについて、このgoroutineも参照して、answerのgoroutineでも isCorrect のメソッドから参照しているのでおそらくraceしてしまいそうです(おそらくまず発生しないと思いますが)
go test -race でテストを実行してみてください

おそらく発生する流れ

aCh <- ansが実行される

mainのforのblockが次に進むので、qChから読み込まれ、c.currentWordが更新される

isCorrectで次の問題の答えと比較されて、間違いと判定されてしまう

おそらくまず発生しないと思いますが

テストで固定で複数行のstringをinputさせて動作させてみたら結果に揺れがあったので、なんでだろうと思っていましたが
多分こいつですね。。。普通に起きていました

mizkei · 2018-11-26T05:09:23Z

kadai3-2/src/tomoyukikobayashi/main.go

+		}
+	}
+	// tmp配下にハッシュにマッチするフォルダがなければ作る
+	dlTmp := filepath.Join("tmp", hash)


ioutilにTempDirというメソッドがあります
https://golang.org/pkg/io/ioutil/#TempDir

ありがとうございます

mizkei · 2018-11-26T05:10:20Z

kadai3-2/src/tomoyukikobayashi/main.go

+			}
+
+			// 取得したデータをBufに読み出し
+			buf, err := ioutil.ReadAll(dRes.Body)


ReadAllせずに io.Copy をすれば十分かと思います

ここも、ストリームはストリームのまま渡そう、という話ですね
ありがとうございます。

mizkei · 2018-11-26T05:13:58Z

kadai3-2/src/tomoyukikobayashi/main.go

+		for i := 0; i < procs; i++ {
+			file, err := os.Open(filepath.Join(dlTmp, strconv.FormatInt(int64(i+1), 10)))
+			// 閉じるの失敗しても実害ないので、エラー処理なし
+			defer file.Close()


これは if err != nil {...} の下にあったほうがよいです
err != nil の場合、Openの実行にエラーがあったということなので、fileが開かれていないと考えたほうが良いです
まずエラーをチェックして、エラーがなければ、fileを触る、としたほうが安全です

ありがとうございます。
ここは単純にミスです。。。

mizkei · 2018-11-26T05:14:23Z

kadai3-2/src/tomoyukikobayashi/main.go

+			files[i] = file
+		}
+		reader := io.MultiReader(files...)
+		b, err := ioutil.ReadAll(reader)


ここも io.Copy で良いと思います

bakisunsan · 2018-11-26T06:33:11Z

色々ためになるコメントありがとうございました 🙇 @mizkei san @knsh14 san

bakisunsan · 2018-11-27T12:52:45Z

レビュー指摘事項に対応。下記はまだ未対応
kadai3-1
ゲーム結果の[2]intにtypeをつける
raceの解消(mutex使うのがいいかな)
テストのTODO修正

kadai3-2
一時ディレクトリをioutil.TempDirで作る

Tomoyuki Kobayashi and others added 26 commits November 8, 2018 15:52

add kadai1 files :lol:

8a7dcf1

add todos

ab31386

add kadai2 files

ef36126

Delete .gitignore

c9587ee

Delete Makefile

e69b664

Delete README.md

9e5a807

Delete main.go

f9f7c4e

Delete file_finder.go

4bf0dd9

Delete formats.go

71569f9

Delete formats_test.go

4cec624

Delete image_converter.go

4458643

Update main.go

d7df320

add load question logics

e5de4ee

とりあえず一通り動くバージョン

06ad2ec

コメント修正

2abbf61

Delete Makefile

3e212f5

Delete README.md

8bfcc95

Delete file_finder.go

1b37ffa

Delete file_finder_test.go

788ceb5

Delete formats.go

bf5cb5e

Delete formats_test.go

26db9b9

Delete image_converter.go

74dd45a

Delete image_converter_test.go

c074309

Delete main.go

d8e8288

Delete main_test.go

06c5785

Update Makefile

00001a7

knsh14 reviewed Nov 22, 2018

View reviewed changes

Tomoyuki Kobayashi added 3 commits November 22, 2018 22:57

refactor game

4f8b436

リファクタリングとテスト追加

89975a8

Merge branch 'kadai3-tomoyuki.kobayashi' of https://github.com/bakisu…

feb6bd9

…nsan/dojo4 into kadai3-tomoyuki.kobayashi

Tomoyuki Kobayashi added 6 commits November 23, 2018 02:36

rename

4b10c87

add test

3dbe9fb

rangeの動作確認ができてる版

5decc01

rangeダウンロードできる版

1a4401c

work version

d4abbe7

work version

ce761c1

bakisunsan changed the title ~~[WIP] Kadai3 tomoyuki.kobayashi~~ Kadai3 tomoyuki.kobayashi Nov 24, 2018

bakisunsan and others added 5 commits November 24, 2018 23:51

Delete file4.txt

7a6cd63

Delete invalid.jpg

28b3f22

ファイルクローズが漏れていたので追加

acefdc7

Merge branch 'kadai3-tomoyuki.kobayashi' of https://github.com/bakisu…

847d7c4

…nsan/dojo4 into kadai3-tomoyuki.kobayashi

add defer

e06f9a4

bakisunsan commented Nov 25, 2018

View reviewed changes

mizkei reviewed Nov 26, 2018

View reviewed changes

fix for review comments

67258fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kadai3 tomoyuki.kobayashi #36

Kadai3 tomoyuki.kobayashi #36

bakisunsan commented Nov 22, 2018 •

edited

Loading

knsh14 Nov 22, 2018

bakisunsan Nov 22, 2018

bakisunsan Nov 25, 2018

mizkei left a comment

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan Nov 26, 2018

mizkei Nov 26, 2018

bakisunsan commented Nov 26, 2018 •

edited

Loading

bakisunsan commented Nov 27, 2018

Kadai3 tomoyuki.kobayashi #36

Are you sure you want to change the base?

Kadai3 tomoyuki.kobayashi #36

Conversation

bakisunsan commented Nov 22, 2018 • edited Loading

課題3

タイピングゲームを作ろう

分割ダウンロードを行う

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mizkei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakisunsan commented Nov 26, 2018 • edited Loading

bakisunsan commented Nov 27, 2018

bakisunsan commented Nov 22, 2018 •

edited

Loading

bakisunsan commented Nov 26, 2018 •

edited

Loading