tanaka0325 / 課題1 - #6
Conversation
|
他の方の PR へのレビューを見て気づいたのですが、テスト画像は （追記） |
39388ed to
5868629
Compare
|
サブディレクトリの画像も対象にしているのですが、場合によってはハチャメチャな量の画像が対象になってしまう可能性があるので、 |
|
|
||
| ## 感想 | ||
|
|
||
| - long option(?) はどうやってやれば良いのだろうか |
There was a problem hiding this comment.
改訂2版 みんなのGo言語に、以下の様な形で書かれていました！
const defaultPort = 3000
var (
port string
)
func init() {
flag.StringVar(&port, "port", defaultPort, "port to use")
flag.StringVar(&port, "p", defaultPort, "port to use (short)")
flag.Parse()
}
上記のような冗長な記述を避けたりする等の目的で、サードパーティ製のパッケージも出ている様です。
私はまだ利用したことないのですが、ご参考までに！
ab9078e to
6c9f2bb
Compare
52b060f to
331d78e
Compare
331d78e to
a235d16
Compare
| package imgconv | ||
|
|
||
| // Args is type for command line arguments. | ||
| type Args []string |
There was a problem hiding this comment.
imgconvパッケージはコマンドライン前提のパッケージでしょうか？
mainパッケージのみがコマンドラインに依存する形をとったほうが良いです。
| var args imgconv.Args | ||
|
|
||
| func init() { | ||
| options.From = flag.String("f", "jpg", "file extension before convert") |
There was a problem hiding this comment.
コマンドラインから貰ってきたデータをそのまま（チェックせずに）設定するのは避けたほうがよいです。
ユーザが入力するデータには予期せぬものもあります。
予期せぬ入力をガードするのはmainパッケージの仕事の方がよいでしょう。
| os.Exit(1) | ||
| } | ||
|
|
||
| os.Exit(0) |
| Encoder | ||
| } | ||
|
|
||
| type CnvImage struct{} |
There was a problem hiding this comment.
この型は何のために定義していますか？
CnvImagePNGなどがCnvImageを継承するイメージであれば、Goには継承は存在しないので不要です。
empty structを基底型した型をさらに基底型とするのはあまり意味がないですね。
| // CnvImagePng is type for png format. | ||
| type CnvImagePNG CnvImage | ||
|
|
||
| func (ip CnvImagePNG) Decode(r io.Reader) (image.Image, error) { return png.Decode(r) } |
| func newCnvImage(ext string) DecodeEncoder { | ||
| switch ext { | ||
| case "png": | ||
| return &CnvImagePNG{} |
There was a problem hiding this comment.
empty struct（struct {}）を基底型にしている場合はポインタを取る必要ない（取らないほうが良い）です。
| "strings" | ||
| ) | ||
|
|
||
| var allowedExts = []string{"png", "jpg", "jpeg", "gif", "bmp", "tiff", "tif"} |
There was a problem hiding this comment.
mapの方が無駄にforで回らなくて良いかなと思います。
詳細は他の方のレビューコメントをご確認ください。
| // convert | ||
| for _, path := range paths { | ||
| filename := strings.Replace(path, "."+fromExt, "", -1) | ||
| // ")) |
There was a problem hiding this comment.
これは何かのコメントでしょうか？
コメントは人が読むものなので、不要なものはPushする際には消して置くと良いです。
デバッグ用に残したい場合はその旨をコメントに書くとレビュアーには優しいです。
| } | ||
|
|
||
| defer func() { | ||
| err = w.Close() |
There was a problem hiding this comment.
名前付き戻り値につかう変数と関数内で使う変数は分けた方が良いです。
| return nil, fmt.Errorf("%s is not a directory", n) | ||
| } | ||
|
|
||
| if err := filepath.Walk(n, func(path string, info os.FileInfo, err error) error { |
| func getTargetFilePaths(args Args, from string) ([]string, error) { | ||
| uns := args.uniq() | ||
|
|
||
| paths := []string{} |
There was a problem hiding this comment.
長さゼロのスライスを作る必要はありません。
var paths []string でゼロ値のnilを利用しましょう。
appendは第1引数がtyped nilでも問題ありません。
| } | ||
|
|
||
| func isDir(path string) (bool, error) { | ||
| f, err := os.Open(path) |
| return false, err | ||
| } | ||
|
|
||
| fi, err := f.Stat() |
|
|
||
| // Options is type for command line options. | ||
| type Options struct { | ||
| From *string |
There was a problem hiding this comment.
ポインタにせず、flagパッケージの関数にわたす際に &opt.From のようにポインタを渡すと良いでしょう。
3 participants
レビューお願いします！
詳しくは README.md を参照ください🙇♂️