【完了】プラグインのコードレビュー依頼：戦闘勝利時のSVアクター動作を指定するプラグイン

[ムノクラ]

また、ご指導ください。

機能を欲張ったら、「本当にこの書き方で合ってるのかな？」という状態になってしまいました。
内部で Sprite_Actor.prototype.refreshMotion を呼ばないで実現する方法があれば、その方がベターだと思っていたのですが、色々試しているうちにこうなってしまいました。

かなり冗長なコードだと思います。
コードレビューをお願いいたします。

[ムノクラ]

まず、仕様に対して実装方針が適切かどうかのレビューから始めるほうが良いです。
小手先のコーディング技術についてのレビューはその後でしたほうが無駄がありません。

そして、一気に望みの仕様のものを書き上げようとするにはまだ知識や経験が不足されているものと思いますので、一歩ずつコードを動かしながら進められるのが良いでしょう。
今回で言えば、勝利時のモーションを固定の何かに変更するプラグインから始めると良かったのかと思います。

内部で Sprite_Actor.prototype.refreshMotion を呼ばないで実現する方法があれば、その方がベターだと思っていたのですが、色々試しているうちにこうなってしまいました。

元のコードで 'victory' と指定されているところを書き換えるだけ、から始めていれば、このような回り道は不要だったはずです。

Game_Battler の requestMotion メソッドで、バトラーにこういう動きをさせてくれ、というリクエストを発行しています。
Sprite_Actor はそのリクエストを検知してモーションを開始しますが、 Game_Actor はその詳細な実装まで知っている必要はありません。
requestMotion でリクエストを発行さえすれば、あとは Sprite_Actor がよろしくやってくれるからです。

さて、実装を見ると、モーション指定用の変数の値が変更されていた場合に、 Sprite_Actor.prototype.refreshMotion を再定義してしまっています。
Sprite_Actor の refreshMotion メソッドは、 Game_Battler （を継承した Game_Actor） で発行された アクターのモーションを再描画してくれ 、というリクエストを検知した際に呼び出されるメソッドです。
これが再定義されてしまうと、戦闘中のモーションリフレッシュのタイミングで、再定義されたほうが呼び出されてしまい、アクターは勝利モーションの変更先として指定したつもりのモーションを取るようになってしまいます。

最初に戻って、今書いたものを省略しないで書いたものがこれです。
どうやら、これでバグは解消したようです。

これから、うたこさんが書いてくださったものを取り入れてい見ます。

[Plasma Dark]

constをvarに変更しても動きました。
constの方が値を厳密に扱うという認識ですが、先日のレビューでトリアコンタンさんが「どっちかに統一したほうが良い」とおっしゃっていたのと、コアが全てvarを使っているそうなので、どうなんだろ？という疑問はあります。
好みの問題か確認するため、全てのvarをconstに置き換えたらエラーになってしまいました。
やはり、厳密なので使用できない箇所はあるんでしょうか。

varは以前からあった変数宣言の方法ですが、ES2015という比較的新し目な仕様で let と const が追加されました。
トリアコンタンさんが仰っていたのは、新しい記法にするのであれば let と const で統一したほうがきれいである、ということだと思います。
（厳密にはスコープ面で挙動が違ったりするのですが）基本的に var は let で置き換えられます。
最近では var より新しく、挙動が直感的になっていてわかりやすい let を使うべき、とされることが多いようです。
const も新しいですが、こちらは再代入不可という性質を持っています。

再代入は可読性を損なう危険のある行為である上、コード中で必要になるシーンは実は少ないので、再代入不可のconstがよく使われるのです。

[ムノクラ]

最初に戻って、今書いたものを省略しないで書いたものがこれです。
どうやら、これでバグは解消したようです。

これから、うたこさんが書いてくださったものを取り入れてい見ます。

バグは直っていませんでしたが、直せたと思います。
同時にうたこさんが教えてくださったコードの書き方を取り入れました。
多分、これで想定通りに動いていると思うのですが…

再度、ご指導ください！

[Plasma Dark]

とても細かい話をします。

この規模のプログラムなら問題になりませんが、 atai という名前は考え直したほうが良いです。
モーションに対してIDを振っているので、 motionId あたりでしょうか。

motionObject は不変であるはずなので、メソッドの外で定義してしまっても良いかと思います。

motionObjectとmotionKeyの定義文の末尾にセミコロンがありません。

atai の比較は !== としたほうが良いです。

[utako]

まず、仕様に対して実装方針が適切かどうかのレビューから始めるほうが良いです。
小手先のコーディング技術についてのレビューはその後でしたほうが無駄がありません。

まさしくおっしゃるとおりだと思います。
バトラークラスに詳しくないのに口出ししたせいで、適切なレビューができていませんでした。ムノクラさんを混乱させてしまったことをお詫びいたします。

[Plasma Dark]

細かい話をしましたが、それ以外の部分は良いと思います。

[ムノクラ]

とても細かい話をします。

この規模のプログラムなら問題になりませんが、 atai という名前は考え直したほうが良いです。
モーションに対してIDを振っているので、 motionId あたりでしょうか。

motionObject は不変であるはずなので、メソッドの外で定義してしまっても良いかと思います。

motionObjectとmotionKeyの定義文の末尾にセミコロンがありません。

atai の比較は !== としたほうが良いです。

理解が難しかったですが、これで反映されたでしょうか？

atai は最初variableとかつけていたので、紛らわしすぎたのでとりあえずってやったままにしてしまいました。
そりゃ、Valueは値だよ！って感じですよね(笑)

motionObject を外側に出しました。
動作は変化ないので、処理的にこの方がスマートということでしょうか。

!== はより厳密に！ということですよね？

これで、反映されているでしょうか？

[ムノクラ]

まさしくおっしゃるとおりだと思います。
バトラークラスに詳しくないのに口出ししたせいで、適切なレビューができていませんでした。ムノクラさんを混乱させてしまったことをお詫びいたします。

とんでもないことでございます。
自分に全く見たことがない方法を提示くださったことに感謝申し上げます。

ほんの少ししか理解できていないと思いますが、if else が続きそうになった時に思い出し、見返し調べ、より理解を深めたいです。
きっかけをくださって、ありがとうございました。

[Plasma Dark]

これで、反映されているでしょうか？

大変良いと思います。