-
Notifications
You must be signed in to change notification settings - Fork 0
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
hokita / 課題1 #5
base: master
Are you sure you want to change the base?
hokita / 課題1 #5
Conversation
こちらレビューお願いいたします。 |
kadai1/hokita/imgconv/converter.go
Outdated
"path/filepath" | ||
) | ||
|
||
func converterFactory(from string, to string) (*Converter, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from, to string
と書けます
kadai1/hokita/imgconv/converter.go
Outdated
"path/filepath" | ||
) | ||
|
||
func converterFactory(from string, to string) (*Converter, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
型で十分伝わるので名前はnew
で十分かなと思います
kadai1/hokita/imgconv/converter.go
Outdated
func (conv *Converter) Execute(path string) error { | ||
// ignore unrelated file | ||
if !conv.fromImage.IsMatchExt(filepath.Ext(path)) { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これだと成功したのか失敗したのか無視されたのか分かりづらいので、エラーを返して、呼び出し元で判断してもらうのが良さそうですね。たとえば、os.IsNotExist
関数とかみたいにエラーを判定する関数があると良いかも知れないです。
(この辺はエラー処理の章でやります!)
kadai1/hokita/imgconv/converter.go
Outdated
|
||
// output file | ||
out, err := os.Create(conv.SwitchExt(path)) | ||
defer out.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Create
のエラー処理の後で、defer
文でClose
をしてください。
Close
メソッドもエラーを返すので、書き込みの場合のみエラー処理をお願いします。
コマンドラインツールの章のスライドにやり方は書いてあるので確認してみてください!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
書き込みの場合では Close
がエラーを返す可能性があるんですね。
勉強になりました🙇
kadai1/hokita/imgconv/converter.go
Outdated
} | ||
|
||
// output image | ||
conv.toImage.Encode(out, img) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
エンコードするのにエラーが起きることはない?
kadai1/hokita/imgconv/imgconv.go
Outdated
from = flag.String("from", "jpg", "Conversion source extension.") | ||
to = flag.String("to", "png", "Conversion target extension.") | ||
) | ||
flag.Parse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main
パッケージ以外でflag
パッケージに依存させない(明示的に*flag.FlagSet
を保つ場合を除く)。
flag
パッケージやos.Args
に依存すると、コマンドラインツールありきのパッケージになってしまいます。
kadai1/hokita/imgconv/jpeg_image.go
Outdated
|
||
type JpegImage struct{} | ||
|
||
func (_ JpegImage) Encode(w io.Writer, m image.Image) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レシーバを参照しない場合は_
も省略できます。
kadai1/hokita/imgconv/jpeg_image.go
Outdated
} | ||
|
||
func (_ JpegImage) Extensions() []string { | ||
return []string{".jpg", ".jpeg", ".JPG", ".JPEG"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapの方がいいのと、メソッドを呼び出すたびに生成する必要はないのでパッケージ変数にしても良さそうです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
以下同様です。
kadai1/hokita/main.go
Outdated
) | ||
|
||
const ( | ||
ExitCodeOk int = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iota
は値に意味がなく、区別が付けばいいときのみに使います。
そのため、0
が成功、それ以外が失敗となっている終了コードに使わない方が良いでしょう。
return &Converter{fromImage, toImage}, nil | ||
} | ||
|
||
var ErrUnmatchExt = errors.New("ext does not match") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsNotMatchExt
関数が公開されているのでこちらは公開しないほうが良いです。
|
||
// file open | ||
file, err := os.Open(path) | ||
defer file.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defer Closeはエラー処理のあと
|
||
// output file | ||
out, err := os.Create(conv.SwitchExt(path)) | ||
defer func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defer Closeはエラー処理のあと
|
||
type ImageType interface { | ||
Encode(w io.Writer, m image.Image) error | ||
IsMatchExt(ext string) bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has
とかぐらいでいい気もしますね。
pngImage.Has(ext)
とかになると思うので。
長くてHasExt
とか。
type ImageType interface { | ||
Encode(w io.Writer, m image.Image) error | ||
IsMatchExt(ext string) bool | ||
GetMainExt() string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらも Ext
で十分かなと思います。
} | ||
|
||
func selectImage(ext string) (ImageType, error) { | ||
pngImage := PngImage{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
フィールドの初期化を伴わない構造体リテラル(コンポジットリテラル)での初期化は不要です。
構造体もゼロ値で初期化されるため、var pngImg PngImage
のように変数を定義されば十分です。
} | ||
|
||
err = filepath.Walk(dir, | ||
func(path string, info os.FileInfo, err error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
引数のエラーのエラー処理。
|
||
const QUALITY = 100 | ||
|
||
type JpegImage struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JPEGで十分だと思います。Goでは略語はすべて大文字か全部小文字にします。
"io" | ||
) | ||
|
||
const QUALITY = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
定数であってもキャメルケースで書きます。
先頭が大文字だとパッケージ外に公開されます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JPEGのQUALITYっていうのが分かるようにしたほうが良さそうですね。
課題 1 画像変換コマンドを作ろう
課題内容
次の仕様を満たすコマンドを作って下さい
以下を満たすように開発してください
対応したこと
動作
工夫したこと
image_type
というinterfaceを作ってみた。わからなかったこと、むずかしかったこと