Skip to content

kadai3-2 haijima#54

Open
haijima wants to merge 5 commits intomasterfrom
kadai3-2-haijima
Open

kadai3-2 haijima#54
haijima wants to merge 5 commits intomasterfrom
kadai3-2-haijima

Conversation

@haijima
Copy link
Copy Markdown

@haijima haijima commented Sep 9, 2018

詳細はREADME.mdに記載

@haijima haijima self-assigned this Sep 9, 2018
Copy link
Copy Markdown

@vvakame vvakame left a comment

Choose a reason for hiding this comment

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

テストが書けるとよいですね。
全体的に関数のみで構成されているので、美しいコードとするためにどこかでstruct+methodを使うとよりよくできないか考えてみるとよいでしょう。
他の人に僕が書いたレビューも読んで貰えると嬉しいです(同じような内容は指摘漏れしてたりするので…

Comment thread kadai3-2/haijima/main.go
}
}

func acceptsRangeRequest(url string) (int, bool) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

このパターンの場合、errorを返したほうがよいと思います。
例えばURLがinvalidだった場合など。
握りつぶしているerrのうち、握りつぶすのが妥当なものは無さそうです。

Comment thread kadai3-2/haijima/main.go
}
defer resp.Body.Close()
unit := resp.Header.Get("Accept-Ranges")
length, err := strconv.Atoi(resp.Header.Get("Content-Length"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ここも "" だった場合の処理を別途入れたほうがきれいです。

Comment thread kadai3-2/haijima/main.go

func concat(dst io.Writer, srcs []io.Reader) error {
for _, src := range srcs {
err := func() error {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

こここの関数で囲うのは意味がなさそう?

Comment thread kadai3-2/haijima/main.go
length, ok := acceptsRangeRequest(url)
if num > 1 && ok {
b := length/num + 1
eg, ctx := errgroup.WithContext(context.Background())
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tenntennさんからの講義にあった通りです。

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants