Skip to content

kadai1: make image converter #1

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

sh-tatsuno
Copy link

@sh-tatsuno sh-tatsuno commented May 28, 2019

I have implemented go-converter called "go photo".

  • convert between .jpeg, .png, .gif
  • add arg about the maximum number of converting files

It would be appreciated if you review my code.

The points especially I want to check

  • The folder construction
  • The division of functions
  • How much should I coverage test
  • The efficiency of codes I have written

@sh-tatsuno sh-tatsuno requested a review from tenntenn May 28, 2019 14:08
ext := filepath.Ext(path)
var err error
err = func(ext string) error {
for _, postfix := range []string{".jpeg", ".jpg", ".gif", ".png"} {
Copy link

@ebiiim ebiiim Jun 1, 2019

Choose a reason for hiding this comment

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

IMHO: postfixは間違いではありませんが、suffixのほうがより伝わりやすいかもしれません。ご参考

(同名のメールサーバがありますので、ちょっと気になっちゃいました)

Copy link
Author

Choose a reason for hiding this comment

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

たしかにおっしゃる通り...、ご指摘ありがとうございます!

}

// Replace replace image
func (i *Img) Replace(ext string) error {
Copy link

Choose a reason for hiding this comment

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

メソッド名から処理(画像変換)を想像できないかもです。Convert(ext string, removeOldFile bool)などいかがでしょうか?

Copy link
Author

Choose a reason for hiding this comment

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

これもたしかにおっしゃる通りですね...!

var dirName, input, output string

// args
flags := flag.NewFlagSet("imgconv", flag.ContinueOnError)

Choose a reason for hiding this comment

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

本質的でない細かなコメントなのですが、”imgconv” のリテラルは os.Args[0] で表現した方がより再利用性が高まると思います。

Copy link
Author

Choose a reason for hiding this comment

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

なるほど...!ご指摘ありがとうございます!


import (
"image"
"image/gif" // import gif
Copy link
Member

Choose a reason for hiding this comment

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

コメントがコードから分かる以上の情報がないんですがコメントを残した理由はなんでしょうか?

)

// Img image struct
type Img struct {
Copy link
Member

Choose a reason for hiding this comment

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

略さない

Copy link
Member

Choose a reason for hiding this comment

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

Pathと画像自体の情報を持ったものがImgで良いのか?

}

// NewImg generate Img struct
func NewImg(path string) (*Img, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Img構造体を作ること以上のことをやっている。

}

// Save save img for input format if format satisfies ".jpeg", ".jpg", ".gif", ".png"
func (i *Img) Save(path string) error {
Copy link
Member

Choose a reason for hiding this comment

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

せっかくフィールドにPathを持っているのになぜ引数で渡すのか?

func (i *Img) Save(path string) error {
ext := filepath.Ext(path)
var err error
err = func(ext string) error {
Copy link
Member

Choose a reason for hiding this comment

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

クロージャ内で参照しないのであれば、err := ...でよい。


const (
// ExitCodeOK : exit code
ExitCodeOK = iota
Copy link
Member

Choose a reason for hiding this comment

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

外部に出る値でiotaを使うのはあまりよくない。
順番が変わったり間に増えたら値が変わる。


if dirName == "" {
usage()
fmt.Printf("Expected dir path\n")
Copy link
Member

Choose a reason for hiding this comment

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

os.Stdoutに出さない

var pathList []string
pathList, err := dir.Lookup(dirName, input, pathList)
if err != nil {
fmt.Printf("can not open file, %v\n", err)
Copy link
Member

Choose a reason for hiding this comment

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

os.Stdoutに出さない

}

// lookup
var pathList []string
Copy link
Member

Choose a reason for hiding this comment

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

変数定義せずに[]string{}のようなリテラルを引数に渡したほうがわかりやすい。

}

if err := img.Convert(output); err != nil {
fmt.Printf("can not convert file, %v\n", err)
Copy link
Member

Choose a reason for hiding this comment

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

os.Stdoutに出さない。

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.

4 participants