Kadai3 2 kimuson13 #50
Conversation
| parallel := flg.Int("p", runtime.NumCPU(), "separate Content-Length with this argument") | ||
| timeout := flg.Int("t", 30, "timeout for this second") | ||
| filename := flg.String("f", "paralleldownload", "save the file as this name") | ||
| flg.Parse(args) |
There was a problem hiding this comment.
失敗するケースを再現できなかったのですが、errorを返す関数であるのでエラー処理しておいた方が良さそうです。
| } | ||
|
|
||
| acceptRange := res.Header.Get("Accept-Ranges") | ||
| fmt.Fprintf(os.Stdout, "got: Accept-Ranges: %s\n", acceptRange) |
There was a problem hiding this comment.
Fprintfもerrorを返す巻数なので、エラー処理しておいた方が良いです
| if err != nil { | ||
| return err | ||
| } | ||
| sub.Close() |
There was a problem hiding this comment.
ここでsub.Closeを呼ぶと上でエラーになったときにクローズされない可能性があります。そのため
sub, err := os.Open(n)
if err != nil {
return err
}
defer sub.Close()
このようにエラー処理の直後にdeferで閉じ処理をするまでが基本的な書き方です。
ただし今回のようにループでdeferを使うのは問題があるケースが多いです。最後にsubに対してループ回数分deferが実行されるので意図しない動作になります。なので、結論としてループの中の処理を関数に切り出してその中でdeferを使うのが適切かと思います。
| if err != nil { | ||
| return err | ||
| } | ||
| defer file.Close() |
| if err != nil { | ||
| return err | ||
| } | ||
| defer fh.Close() |
There was a problem hiding this comment.
エラー周りで複数指摘を入れていますが、これらを見つけるためにgithub.com/kisielk/errcheckというツールを使っています。
[~/go/src/github.com/kimuson13/gopherdojo-studyroom/kadai3-2/kimuson13] % errcheck ./...
download/download.go:77:13: fmt.Fprintf(os.Stdout, "Start HEAD request to check Content-Length\n")
download/download.go:91:13: fmt.Fprintf(os.Stdout, "got: Accept-Ranges: %s\n", acceptRange)
download/download.go:100:13: fmt.Fprintf(os.Stdout, "got: Content-Length: %v\n", contentLength)
download/download.go:127:19: defer file.Close()
download/download.go:164:13: fmt.Fprintf(os.Stdout, "start GET request: bytes=%d-%d\n", r.low, r.high)
download/download.go:170:22: defer res.Body.Close()
download/download.go:180:20: defer output.Close()
download/download.go:197:16: defer fh.Close()
download/download.go:210:12: sub.Close()
download/option.go:23:11: flg.Parse(args)
deferでのエラーハンドリングは少し工夫が必要な上、おきてもログに出すぐらいしかできないケースが多いのですが、それらに埋もれて本当にハンドルしなきゃいけないものが漏れるよりかは指摘がゼロになるように直した方が早いかと思うのでそうすることをお勧めします。
以下のような形で無名関数を使えば良いので参考にしてください。
https://joniy-joniy.hatenablog.com/entry/2017/12/31/182004
| //Preparate method define the variables to Donwload | ||
| func (d *Downloader) Preparate() error { | ||
| if d.parallel < 1 { | ||
| d.parallel = 2 |
There was a problem hiding this comment.
flagでデフォルト runtime.NumCPU()になっているのでそれに揃えるか、この関数自体をバリデーション用の関数として(名前もValidationとかに)、その上で1未満だとエラーを返すとかの方が良いのではと思います。
| } | ||
|
|
||
| if d.timeout < 1 { | ||
| d.timeout = 5 |
There was a problem hiding this comment.
ここもflagでは30sなので、そちらに揃える方が自然です。
かつ数字を直接書くよりは定数にしてflagの方もそれを使うのが良いでしょう。
| } | ||
|
|
||
| //Preparate method define the variables to Donwload | ||
| func (d *Downloader) Preparate() error { |
There was a problem hiding this comment.
一応現状ソースのままならエラーが返ることは無いので、エラーを返す関数にする必要がないですね
kadai3-2/kimuson13/main.go
Outdated
| if err := os.RemoveAll("tempdir"); err != nil { | ||
| fmt.Println("Error:", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
tempdirの削除はdownload.goに持たせるべきなので、そこにまとめましょう
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
Downloadでtempdirの作成、MergeFileでtempdirの削除を行なっていますが、エラーになったときにtmpdirが消されないケースがあるので、ここで以下のように作成の関数と削除の関数を作り削除の関数をdeferを使って呼ぶと良いです
if err := createTmpDir(); err != nil {
return err
}
defer deleteTmpDir()
|
Downloadのテストはファイルをダウンロードして、それがいろいろな条件のもと意図したファイルが落とせているかを検証すれば良さそうです。その際におそらく困るのがURLで、使っていい外部のURLがあったとしても毎回テストの度にアクセスが走ることやテストのURLにアクセスできる環境を担保しなければならないなどの問題があります。URLが用意できればいいということは、サーバがあればいいということなのでダミーサーバを作れればやりたいことができそうです。 ダウンロードできるまでは試しに書いてみました。（拡張子はgoに直してください、githubでそのまま送れなかったので） 同じようにやっているケースとして、私が参加したGopher道場三回でよく書けているケースを貼っておきます。参考にしてください！ |
課題3-2 分割ダウンローダを作ろう
pgetを参考にしました。ダウンロードのテストを書くのがうまくいかなかったです。それについて教えていただけると助かります。Resetという名前のコミット以降から見てください。