Kadai1,2 kotaaaa #87

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

Jump to bottom

Open

kotaaaa wants to merge 13 commits into gopherdojo:master

from kotaaaa:kadai1-kotaaaa

Open

Kadai1,2 kotaaaa #87

kotaaaa wants to merge 13 commits into gopherdojo:master from kotaaaa:kadai1-kotaaaa

Conversation

@kotaaaa

Copy link

@kotaaaa kotaaaa commented Nov 4, 2021 •

edited

Loading

課題1を作成しました。
使い方については、READMEに記載しています。

お時間あれば、レビューをよろしくお願いします。

課題1

次の仕様を満たすコマンドを作って下さい
- ディレクトリを指定する
- 指定したディレクトリ以下のJPGファイルをPNGに変換(デフォルト)
- ディレクトリ以下は再帰的に処理する
- 変換前と変換後の画像形式を指定できる(オプション)
以下を満たすように開発してください
- mainパッケージと分離する
- 自作パッケージと標準パッケージと準標準パッケージのみ使う
- 準標準パッケージ:golang.org/x以下のパッケージ
- ユーザ定義型を作ってみる
- Go Modulesを使ってみる

2021年11月06日追記
課題2 の単体テストの追加

kotaaaa added 11 commits

November 3, 2021 18:09

@kotaaaa


 First commit

8d314f1

@kotaaaa


 Set directory

48d542a

@kotaaaa


 Add path

6362fea

@kotaaaa


 Add TODO

f9e6e91

@kotaaaa


 until recursive

323ac4a

@kotaaaa


 Cleaned

f54bca3

@kotaaaa


 Fix README

3bf9772

@kotaaaa


 Use user create type

ece43ce

@kotaaaa


 Fix comment

@kotaaaa


 Until user type create

6ed4ab3

@kotaaaa


 Until unit test

21a2937

@kotaaaa kotaaaa changed the title ~~(削除) Kadai1 kotaaaa (削除ここまで)~~ (追記) Kadai1,2 kotaaaa (追記ここまで)

Nov 6, 2021

gosagawa

gosagawa reviewed

Nov 10, 2021

View reviewed changes

kadai1/kotaaaa/search/search.go Outdated

// Get the files in the target directory

files, err := ioutil.ReadDir(dir)

if err != nil {

log.Fatal(err)

Copy link

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

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

エラーはここで処理するのではなく、この関数を

func GetFiles(dir string, ext string) ([]string,error) {

とした上で呼び出し元で処理した方が良いです。理由としてはそのようにエラーハンドリングするのがGoの一般的な書き方であるほか、以下のような問題があるためです

エラーのハンドリングを呼び出し元でコントロールできない
この関数をCLI以外で使うときに中断されると困るケースがある
ログを出したくないときに使いづらい
log.Fatal(err)は内部でos.Exit(1)を呼び出すため、main()関数以外で使用すると他のゴルーチンの終了を待たずして大元であるmain()ゴルーチンを終了させてしまう

kadai1/kotaaaa/search/search.go Outdated

name := file.Name()

// If the file is directory, add files recursively.

if file.IsDir() {

for _, subFile := range GetFiles(dir+name, ext) {

Copy link

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

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

パスの結合はfilepath.Joinを使う方が良いでしょう。ディレクトリの区切りが/以外のプラットフォームでも対処できますし、ディレクトの最後に/がつくつかないで誤動作することがなくなります

kadai1/kotaaaa/search/search_test.go Outdated

for _, ext := range extentions {

files := GetFiles("../testdata/", ext)

for _, file := range files {

if filepath.Ext(file) != ext {

Copy link

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

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

ここではGetFilesが意図どおりに動くかのテストをすべきかと思うので、拡張子が想定通りかを調べるよりファイル名が想定通りかを調べるほうが望ましいかと思います

kadai1/kotaaaa/search/search.go Outdated

log.Fatal(err)

}

var arr []string

Copy link

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

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

細かいところになりますが、[]stringはarrayではなくsliceです([3]stringはarrayです)。変数名はarrではなく、slやlist,意味ある名前にするならtargetFilesとかresultsにするのが適切かと思います。変数名をどれにすべきかは場合によるのですが、とりあえずarrayは適切でなさそうとことを覚えておいてもらえると良さそうです

kadai1/kotaaaa/validator/validator.go

}

if !validateFileFormat(targetSrcExt) || !validateFileFormat(targetDstExt) {

return errors.New("Error: Invalid or Unsupported file format")

}

Copy link

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

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

入力と出力でどちらが問題なのかわかるように分けておいた方が親切かと思います

kadai1/kotaaaa/validator/validator_test.go Outdated

}{

{"Success1", "../testdata", ".png", ".jpg", false},

{"Success2", "../testdata", ".jpg", ".gif", false},

{"Success2", "../testdata", ".gif", ".png", false},

Copy link

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

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

名前が同じだとどちらがエラーとなったときにわかりづらいほか、1,2というように番号を振ってくと途中に追加するときに煩雑だったりどの番号がどのケースかがわかりづらいのでcaseNameを以下のようにわかりやすくつけとくと良いかと思います。
png to jpg
jpg to gif
jpg to svg

@gosagawa

Copy link

gosagawa commented Nov 10, 2021

kadai1/kotaaaa/converterにビルド済みのファイルがあげらていますが、これは環境毎に異なるものになりますし、ファイルサイズも大きいのでgitに含めない方が良さそうです

kotaaaa added 2 commits

November 10, 2021 22:21

@kotaaaa


 Fix reviewed point by gosagawa san

f74a772

@kotaaaa


 using pringln not log.fatal

1b34df3

@kotaaaa

Copy link

Author

kotaaaa commented Nov 10, 2021

@gosagawa さん
丁寧にレビュー頂きありがとうございます!
一通り修正しました。勉強になりました。mm

Labels

None yet

2 participants

@kotaaaa @gosagawa

Kadai1,2 kotaaaa #87

Are you sure you want to change the base?

Kadai1,2 kotaaaa #87

Uh oh!

Conversation

@kotaaaa kotaaaa commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

@gosagawa gosagawa Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

gosagawa commented Nov 10, 2021

Uh oh!

kotaaaa commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

@kotaaaa kotaaaa commented Nov 4, 2021 •

edited

Loading