OpenPNE 3

修正コミットが見当たらないです．

フィードバック¶

すべて save() メソッドの中に記述されていますが，この辺りは「Orientation にしたがって画像を回転する」という明確な目的の一連の流れが存在するため，名前付けしてメソッドとして抽出してもらったほうがよいです．

メソッド抽出の具体的なメリットは リファクタリングという書籍の 「Extract Method（メソッドの抽出）」の項目が参考になります．

本チケットにおけるメソッド抽出の利点としては #3561 の修正と同時にマージする際に save() 内でのコンフリクトの解消時に ロジック記述部分を気にしたマージを行う必要が生じるものを，メソッド抽出後は save() 内ではメソッドの追加のみで対応できるというようなことがあります．

実際にマージを行う場合では画像変更処理はまとめておいたほうがよいと思われるためメソッドに抽出した処理については共通化する可能性はありますが，処理単位でまとまっているほうが共通化なども行いやすくなります．

https://github.com/openpne/OpenPNE3/pull/107 でプルリクエストが来ています

だいぶよくなったと思います．以下簡単にフィードバック．
ちなみに github 側の pull request についてですが，今の段階ではコミットを一つにまとめる必要はないです．
この段階で一つにまとめてしまうとフィードバックの修正内容が最初のコミットからどのように反映されているか，あるいはそもそも反映されているかどうかが追いにくいためです．あと，念のため先んじて書いておきますが「fixed for feedback」のような修正内容がわからないコミットメッセージは後々コードを git log や git blame などで追う際に重大な情報量不足となるため控えてもらうと助かります．

フィードバック1¶

アクセス修飾子がすべて public になっていますが外側で使うタイミングがなさそうなメソッドのため private または protected のほうが良いです．ここの場合， File オブジェクトは部分集合として画像を持ち，その部分集合の要素である場合に必要なメソッドであるため外側のクラスからの呼び出しで用いられることは望ましくなく private で良いと思います． public や protected にするならば画像または画像変換ロジックを扱うクラスで行ったほうがよさそうです．
一般的にプロパティやメソッドのスコープは必要でない限り小さいほうが良いというスタンスが多いと思います．具体的な例は検索してもらうとよいと思います．

フィードバック2¶

createOrientedImage() 内の case 文で 3, 6, 8 などマジックナンバーが用いられています．今の時点ではコードを見ればまあわかりますが，より早く処理の概要を捉える目的やコードが肥大化した場合に処理が追いにくくなること防ぐ目的のために回転方向をコメントで追記してもらうと良いです．またコーディング規約には記述されていませんがコメントで単語や文章を用いる際には英語でお願いします．

フィードバック3¶

コーディング規約について

http://www.openpne.jp/coding-standards-ja/#id28

クラスと同様、波括弧は関数名の次に書かなければなりません。関数名と引数定義用の開始括弧の間にはスペースを入れてはいけません。

http://www.openpne.jp/coding-standards-ja/#id34

変数と値を比較する際は最初に値を置き、場合によっては型チェックもおこなってください:

if (1 === $var)

対応有難うございました．大丈夫と思うためマージします．

更新履歴 5d3ca7b953f42e04462aefeede9862233904410a で適用されました。

レビュー待ちに戻ってしまっているのでステータス変更します。

OpenPNE 3.8.11 にて対応済みであったため、対応せずとします。