Kadai3 2 gosagawa #58
Conversation
|
dep 使ってる偉い…！！ |
|
|
||
| func main() { | ||
|
|
||
| url := os.Args[1] |
There was a problem hiding this comment.
この後に TODO check args とあるので自覚されている気もしますが、どのような引数が与えられた(与えられなかった)場合でもpanicにならないようにしましょう。
|
|
||
| d, err := NewDownloader(url) | ||
| if err != nil { | ||
| fmt.Printf("\ndownload initialize error. %v", err) |
| return http.DefaultClient.Do(req) | ||
| } | ||
|
|
||
| func getFileName(resourceUrl string) string { |
There was a problem hiding this comment.
自力で文字列操作するよりも、既存のものが既に準備されていないか調べるようにしましょう。
go で path を操作したいので、go path と検索してみます。
すると、 https://golang.org/pkg/path/ が見つかり、準備されている関数を調べていくと https://golang.org/pkg/path/#Base が妥当そうだな、とわかります。
| return nil, err | ||
| } | ||
|
|
||
| size := uint(res.ContentLength) |
| return d, nil | ||
| } | ||
|
|
||
| func NewWorker(d *Downloader, size uint, i uint, split uint, url string) (*worker, error) { |
There was a problem hiding this comment.
この関数はpackage外部にexportするべきかどうか考えてみましょう(しなくてよさそう
| bytesToFinishReading = size | ||
| } | ||
| w := &worker{ | ||
| processId: i, |
There was a problem hiding this comment.
プロセスIDと言われると別のものを想像するので、単に id か index が妥当な名前かもしれません
| if err != nil { | ||
| return errors.Wrap(err, "failed to open part file") | ||
| } | ||
| io.Copy(outputFile, partFile) |
There was a problem hiding this comment.
io.Copy はerrを返すので、無視せずにちゃんとチェックするようにしましょう。
https://golang.org/pkg/io/#Copy
errを無視している場合、それを怒ってくれるツールがあった気がします( go vet だっけ…？
いちいち調べるのがめんどくさい場合、そういうツールの助けを得るのはいい考えです
| return errors.Wrap(err, "failed to open part file") | ||
| } | ||
| io.Copy(outputFile, partFile) | ||
| partFile.Close() |
There was a problem hiding this comment.
Openした後に defer partFile.Close() とすることで、Close漏れを防ぐことができます。
"正しく処理する" ことを努力するより、 "嫌でも正しくなってしまう" 習慣を身につけるほうが後々楽ができます。
|
@vvakame レビューどうもありがとうございます！ |
自力で作りきるのが厳しく、他の方の実装を参考にしつつ最低限ダウンロードまではできるように実装しました。
キャンセルが発生した場合の実装はできていません。
引き続きブラッシュアップしますが、現状の状態で申請いたします。お願いします。