Skip to content
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

tanaka0325 / 課題1 #6

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions kadai1/tanaka0325/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# Image Converter

## Spec

```
## 次の仕様を満たすコマンドを作って下さい

- ディレクトリを指定する
- 指定したディレクトリ以下のJPGファイルをPNGに変換(デフォルト)
- ディレクトリ以下は再帰的に処理する
- 変換前と変換後の画像形式を指定できる(オプション)

## 以下を満たすように開発してください

- mainパッケージと分離する
- 自作パッケージと標準パッケージと準標準パッケージのみ使う
- 準標準パッケージ:golang.org/x以下のパッケージ
- ユーザ定義型を作ってみる
- GoDocを生成してみる
- Go Modulesを使ってみる
```

## Usage

```zsh
# build
$ cd imgconv
$ go build -o imgconv ./cmd/imgconv

# display help
$ ./imgconv -h
Usage of ./imgconv:
-f string
file extention before convert (default "jpg")
-n dry run
-t string
file extention after convert (default "png")

# single directory
$ ./imgconv testdata/images

# multi directories
$ ./imgconv testdata/images testdata/images2

# customize ext
$ ./imgconv -f png -t gif testdata/images

# dry run
$ ./imgconv -n testdata/images testdata/images2
testdata/images/sample1.jpg => testdata/images/sample1.png
testdata/images2/img/sample3.jpg => testdata/images2/img/sample3.png
testdata/images2/sample2.jpg => testdata/images2/sample2.png

```

## 感想

- long option(?) はどうやってやれば良いのだろうか
Copy link

Choose a reason for hiding this comment

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

改訂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()
}

上記のような冗長な記述を避けたりする等の目的で、サードパーティ製のパッケージも出ている様です。
私はまだ利用したことないのですが、ご参考までに!

Copy link
Author

Choose a reason for hiding this comment

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

おお!!ありがとうございます!!!すごくありがたいです🙏

- そもそも作りとしてこれで良いのだろうか・・・めっちゃ悩みました

19 changes: 19 additions & 0 deletions kadai1/tanaka0325/imgconv/args.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package imgconv

// Args is type for command line arguments.
type Args []string
Copy link
Member

Choose a reason for hiding this comment

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

imgconvパッケージはコマンドライン前提のパッケージでしょうか?
mainパッケージのみがコマンドラインに依存する形をとったほうが良いです。


func (args Args) uniq() []string {
m := map[string]bool{}
u := []string{}

for _, v := range args {
if !m[v] {
m[v] = true

u = append(u, v)
}
}

return u
}
30 changes: 30 additions & 0 deletions kadai1/tanaka0325/imgconv/cmd/imgconv/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package main

import (
"flag"
"fmt"
"os"

"github.com/gopherdojo/dojo8/kadai1/tanaka0325/imgconv"
)

var options imgconv.Options
var args imgconv.Args

func init() {
options.From = flag.String("f", "jpg", "file extension before convert")
Copy link
Member

Choose a reason for hiding this comment

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

コマンドラインから貰ってきたデータをそのまま(チェックせずに)設定するのは避けたほうがよいです。
ユーザが入力するデータには予期せぬものもあります。
予期せぬ入力をガードするのはmainパッケージの仕事の方がよいでしょう。

options.To = flag.String("t", "png", "file extension after convert")
options.DryRun = flag.Bool("n", false, "dry run")
flag.Parse()

args = flag.Args()
}

func main() {
if err := imgconv.Run(options, args); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}

os.Exit(0)
Copy link
Member

Choose a reason for hiding this comment

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

0の場合は明示的に実行する必要ないです。

}
81 changes: 81 additions & 0 deletions kadai1/tanaka0325/imgconv/cnv_image.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package imgconv

import (
"image"
"image/gif"
"image/jpeg"
"image/png"
"io"

"golang.org/x/image/bmp"
"golang.org/x/image/tiff"
)

type Decoder interface {
Decode(io.Reader) (image.Image, error)
}

type Encoder interface {
Encode(io.Writer, image.Image) error
}

type DecodeEncoder interface {
Decoder
Encoder
}

type CnvImage struct{}
Copy link
Member

Choose a reason for hiding this comment

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

この型は何のために定義していますか?
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) }
Copy link
Member

Choose a reason for hiding this comment

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

レシーバ変数を使わない場合は省略が可能です。


func (ip CnvImagePNG) Encode(w io.Writer, i image.Image) error { return png.Encode(w, i) }

// CnvImageJPEG is type for jpeg format.
type CnvImageJPEG CnvImage

func (ip CnvImageJPEG) Decode(r io.Reader) (image.Image, error) { return jpeg.Decode(r) }

func (ip CnvImageJPEG) Encode(w io.Writer, i image.Image) error { return jpeg.Encode(w, i, nil) }

// CnvImageGIF is type for gif format.
type CnvImageGIF CnvImage

func (ip CnvImageGIF) Decode(r io.Reader) (image.Image, error) { return gif.Decode(r) }

func (ip CnvImageGIF) Encode(w io.Writer, i image.Image) error {
return gif.Encode(w, i, &gif.Options{NumColors: 256})
}

// CnvImageBMP is type for bmp format.
type CnvImageBMP CnvImage

func (ip CnvImageBMP) Decode(r io.Reader) (image.Image, error) { return bmp.Decode(r) }

func (ip CnvImageBMP) Encode(w io.Writer, i image.Image) error { return bmp.Encode(w, i) }

// CnvImageTIFF is type for tiff format.
type CnvImageTIFF CnvImage

func (ip CnvImageTIFF) Decode(r io.Reader) (image.Image, error) { return tiff.Decode(r) }

func (ip CnvImageTIFF) Encode(w io.Writer, i image.Image) error { return tiff.Encode(w, i, nil) }

func newCnvImage(ext string) DecodeEncoder {
switch ext {
case "png":
return &CnvImagePNG{}
Copy link
Member

Choose a reason for hiding this comment

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

empty struct(struct {})を基底型にしている場合はポインタを取る必要ない(取らないほうが良い)です。

case "jpg", "jpeg":
return &CnvImageJPEG{}
case "gif":
return &CnvImageGIF{}
case "bmp":
return &CnvImageBMP{}
case "tiff", "tif":
return &CnvImageTIFF{}
}

return nil
}
5 changes: 5 additions & 0 deletions kadai1/tanaka0325/imgconv/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module github.com/gopherdojo/dojo8/kadai1/tanaka0325/imgconv

go 1.14

require golang.org/x/image v0.0.0-20200618115811-c13761719519
3 changes: 3 additions & 0 deletions kadai1/tanaka0325/imgconv/go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
golang.org/x/image v0.0.0-20200618115811-c13761719519 h1:1e2ufUJNM3lCHEY5jIgac/7UTjd6cgJNdatjPdFWf34=
golang.org/x/image v0.0.0-20200618115811-c13761719519/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
116 changes: 116 additions & 0 deletions kadai1/tanaka0325/imgconv/imgconv.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
// Imgconv package is to convert images file format.
package imgconv

import (
"fmt"
"os"
"path/filepath"
"strings"
)

var allowedExts = []string{"png", "jpg", "jpeg", "gif", "bmp", "tiff", "tif"}
Copy link
Member

Choose a reason for hiding this comment

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

mapの方が無駄にforで回らなくて良いかなと思います。
詳細は他の方のレビューコメントをご確認ください。

var fromExt string
var toExt string

// Run is to convert image file format.
func Run(options Options, args Args) error {
// // validator
if err := options.validate(allowedExts); err != nil {
return err
}

fromExt = *options.From
toExt = *options.To

// get target image flepaths from args
paths, err := getTargetFilePaths(args, fromExt)
if err != nil {
return err
}

// convert
for _, path := range paths {
filename := strings.Replace(path, "."+fromExt, "", -1)
// "))
Copy link
Member

Choose a reason for hiding this comment

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

これは何かのコメントでしょうか?
コメントは人が読むものなので、不要なものはPushする際には消して置くと良いです。
デバッグ用に残したい場合はその旨をコメントに書くとレビュアーには優しいです。

f := newCnvImage(fromExt)
t := newCnvImage(toExt)

if *options.DryRun {
fmt.Printf("%s.%s => %s.%s \n", filename, fromExt, filename, toExt)
} else if err := convert(f, t, filename); err != nil {
return err
}
}

return nil
}

func convert(d Decoder, e Encoder, filename string) (err error) {
// open file
r, err := os.Open(filename + "." + fromExt)
if err != nil {
return
}
defer r.Close()

// decode
img, err := d.Decode(r)
if err != nil {
return
}

// create file
w, err := os.Create(filename + "." + toExt)
if err != nil {
return err
}

defer func() {
err = w.Close()
Copy link
Member

Choose a reason for hiding this comment

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

名前付き戻り値につかう変数と関数内で使う変数は分けた方が良いです。

}()

// encode
if err := e.Encode(w, img); err != nil {
return err
}

return
}

func getTargetFilePaths(args Args, from string) ([]string, error) {
uns := args.uniq()

paths := []string{}
Copy link
Member

@tenntenn tenntenn Jul 20, 2020

Choose a reason for hiding this comment

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

長さゼロのスライスを作る必要はありません。
var paths []string でゼロ値のnilを利用しましょう。
appendは第1引数がtyped nilでも問題ありません。

for _, n := range uns {
if ok, err := isDir(n); err != nil {
return nil, err
} else if !ok {
return nil, fmt.Errorf("%s is not a directory", n)
}

if err := filepath.Walk(n, func(path string, info os.FileInfo, err error) error {
Copy link
Member

Choose a reason for hiding this comment

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

WalkFuncの引数のエラーのエラー処理

if filepath.Ext(path) == "."+from {
paths = append(paths, path)
}
return nil
}); err != nil {
return nil, err
}
}

return paths, nil
}

func isDir(path string) (bool, error) {
f, err := os.Open(path)
Copy link
Member

Choose a reason for hiding this comment

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

Closeしてないですね

if err != nil {
return false, err
}

fi, err := f.Stat()
Copy link
Member

Choose a reason for hiding this comment

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

os.Statで良いかなと思います。
https://golang.org/pkg/os/#Stat

if err != nil {
return false, err
}

return fi.IsDir(), nil
}
37 changes: 37 additions & 0 deletions kadai1/tanaka0325/imgconv/options.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package imgconv

import (
"errors"
"fmt"
"strings"
)

// Options is type for command line options.
type Options struct {
From *string
Copy link
Member

Choose a reason for hiding this comment

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

ポインタにせず、flagパッケージの関数にわたす際に &opt.From のようにポインタを渡すと良いでしょう。

To *string
DryRun *bool
}

func (opt Options) validate(allowList []string) error {
to := strings.ToLower(*opt.To)
from := strings.ToLower(*opt.From)
targetExts := []string{to, from}

for _, e := range targetExts {
if err := include(allowList, e); err != nil {
return fmt.Errorf("%w. ext is only allowd in %s", err, allowList)
}
}

return nil
}

func include(list []string, w string) error {
for _, e := range list {
if e == w {
return nil
}
}
return errors.New(w + " is not allowed")
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.