tsuchinaga / 課題1 #3
Conversation
kadai1/tsuchinaga/.idea/$CACHE_FILE$
Outdated
| <component name="NodePackageJsonFileManager"> | ||
| <packageJsonPaths /> | ||
| </component> | ||
| </project> |
There was a problem hiding this comment.
.ideaディレクトリはIDEの設定ファイル郡なので、個人的にはignoreした方が良いかと思いました！
There was a problem hiding this comment.
ありがとうございます
.ideaに.gitignoreがはいってたのでうまいことやってくれるもんだと思ってました:sweat_smile:
kadai1/tsuchinaga/conv/converter.go
Outdated
| "path/filepath" | ||
| ) | ||
|
|
||
| var validFileTypes = []string{"jpeg", "png"} |
There was a problem hiding this comment.
mapを使った方がシンプルになりますね。
var validFileTypes = map[string]bool{"jpeg": true, "png": true}
func IsValidFileType(fileType string) bool {
return validFileTypes[fileType]
}
kadai1/tsuchinaga/conv/converter.go
Outdated
| // IsDir - pathがディレクトリかどうか | ||
| func IsDir(path string) bool { | ||
| _, err := ioutil.ReadDir(path) | ||
| return err == nil |
There was a problem hiding this comment.
これではディレクトリかどうかは判断できませんね。
読み込めないエラーには単にストレージが壊れていたりする場合も含まれますので。
os.Statとか使ってos.FileInfoを取得してIsDirメソッドで確認した方がいいです。
kadai1/tsuchinaga/conv/converter.go
Outdated
| } | ||
|
|
||
| // ExecConvert - 変換の実行 | ||
| func ExecConvert(dir, src, dest string) { |
There was a problem hiding this comment.
conv.ExecConvertとなり、意味が二重になります。
Goは他のパッケージの関数などを呼ぶ際は必ず（ほぼ）、パッケージ名も合わせて記載するため、それも込みで名前をつける必要があります。
imgconv.Doとかであれえば、意味の重複もなく、十分意味が伝わるでしょう。
kadai1/tsuchinaga/conv/converter.go
Outdated
|
|
||
| files, err := ioutil.ReadDir(dirPath) | ||
| if err != nil { | ||
| log.Printf("ディレクトリ: %sが読み込めなかったためスキップします\n", dirPath) |
There was a problem hiding this comment.
ライブラリとして使うパッケージが標準出力や標準エラー出力にメッセージを出すのはやりすぎかなと思います。
読み込めない場合はちゃんとエラーとして呼び出し元に返し、呼び出し元でどうするか判断してもらったほうが良いです。
kadai1/tsuchinaga/conv/converter.go
Outdated
| func (c converter) convert(path string) { | ||
| f, err := os.Open(path) | ||
| if err != nil { // 開けない | ||
| log.Printf("ファイル: %sが開けなかったためスキップします\n", path) |
kadai1/tsuchinaga/conv/converter.go
Outdated
| log.Printf("変換後ファイル: %sが作成できなかったためスキップします\n", newFilePath) | ||
| return | ||
| } | ||
| defer o.Close() |
kadai1/tsuchinaga/conv/converter.go
Outdated
| } | ||
| defer o.Close() | ||
|
|
||
| err = nil |
There was a problem hiding this comment.
case句でそれぞれreturnした方が分かりやすいかと思います
kadai1/tsuchinaga/conv/converter.go
Outdated
| if err != nil { | ||
| log.Printf("ファイル: %sの変換に失敗しました\n", path) | ||
| } else { | ||
| log.Printf("%s => %s\n", path, newFilePath) |
There was a problem hiding this comment.
logパッケージはデフォルトでは標準エラー出力に出力を行います。
そのため、エラーではない進捗などは標準出力に出力すべきです。
なお、ライブラリなので、進捗の出力はmainパッケージに任せたほうがよいでしょう。
ライブラリ側は進捗が出力できるように工夫するほうが良いです。
kadai1/tsuchinaga/conv/converter.go
Outdated
| // getFileType - 画像ファイルの型を得る | ||
| func getFileType(path string) string { | ||
| f, err := os.Open(path) | ||
| if err != nil { // 開けない |
There was a problem hiding this comment.
エラーは適切にハンドルしてください。
エラーはすでに値なのでエラーを空文字で表現せずに、そのまま多値でreturnしてください。
kadai1/tsuchinaga/main.go
Outdated
|
|
||
| import ( | ||
| "flag" | ||
| "github.com/gopherdojo/dojo8/kadai1/tsuchinaga/conv" |
kadai1/tsuchinaga/main.go
Outdated
|
|
||
| // validation | ||
| if dir == "" { | ||
| log.Fatalln("dirの指定は必須です") |
There was a problem hiding this comment.
log.Fatal系は内部でos.Exit(1)を呼んでいます。
終了コードは自分で設定すべきだと思うので、fmt.Fprintlnとos.Exitを使うようにしましょう。
kadai1/tsuchinaga/main.go
Outdated
| } | ||
|
|
||
| // 変換実行 | ||
| conv.ExecConvert(dir, src, dest) |
There was a problem hiding this comment.
変換を行っているのにエラー処理がmain関数にないのは不自然です。
エラー処理は必ず呼び出し元が行うようにしましょう。
課題1のPRです。レビューをお願いします。