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

[ムノクラ]

また、ご指導ください。

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

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

[utako]

ひとまずswitch文になっている部分をオブジェクトにして行数を削減してみました。テストはしていないのでおかしかったら教えて下さい。
アクターの動作が今後追加や変更される可能性があるなら、プラグインパラメータに配列として保存できるようにするとよいかと思います。

[ムノクラ]

ひとまずswitch文になっている部分をオブジェクトにして行数を削減してみました。テストはしていないのでおかしかったら教えて下さい。
アクターの動作が今後追加や変更される可能性があるなら、プラグインパラメータに配列として保存できるようにするとよいかと思います。

動作確認できました！
この書き方は初めて見ました。
コアがifの分岐だったので、これが普通なのかな…と思って書いていました。
（仕組みはよく分からんが、とにかく凄いことしか分からない）

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

constにすると速度が上がるというようなメリットはあるのでしょうか？

あと、このifの連続を置き換えた手法は何と呼ぶのでしょうか？
参考になる資料ページなどをご存知でしたら、教えていただければ幸いです。

「プラグインパラメータに配列として保存できるようにする」というのも、理解できていません…
申し訳ありませんが、少し踏み込んだ解説をいただければ幸いです。

[ムノクラ]

動作を17で指定したら、下記のエラーになりました。

コード: 全て選択

rpg_managers.js:1949 ReferenceError: $gamevariables is not defined
    at Game_Actor.performVictory (MNKR_SelectWinAction.js:48)
    at rpg_objects.js:5172
    at Array.forEach (<anonymous>)
    at Game_Party.performVictory (rpg_objects.js:5171)
    at Function.BattleManager.processVictory (rpg_managers.js:2635)
    at Function.BattleManager.checkBattleEnd (rpg_managers.js:2617)
    at Function.BattleManager.updateEventMain (rpg_managers.js:2283)
    at Function.BattleManager.updateEvent (rpg_managers.js:2274)
    at Function.BattleManager.update (rpg_managers.js:2244)
    at Scene_Battle.updateBattleProcess (rpg_scenes.js:2265)
SceneManager.catchException @ rpg_managers.js:1949

[utako]

>このifの連続を置き換えた手法は何と呼ぶのでしょうか？

特に名前は知りません。「switch文をオブジェクト(連想配列)で書けるやつ」と呼んでいます(Stateパターンと呼ばれるデザインパターンが近そう)。else ifやswitchで書くより書きやすいし読みやすいしメンテナンス性も高いのでより多くの人に使って欲しいです。

記事もあまり見つかりませんでしたが、こちらなど。
http://naoyashiga.hatenablog.com/entry/ ... /29/140528

https://pisuke-code.com/javascript-switch-alternatives/
こちらの記事ではオブジェクトの値に変数ではなく関数を入れていますね。
この手法は自由度が高くないのが欠点ですが、関数を入れられるようになると使い勝手がグッと上がります。



>constにすると速度が上がるというようなメリットはあるのでしょうか？

constで変数を宣言すると、変数の値を上書きできなくなるので厳密性が上がります。
厳密になることのなにがいいのかというと、

`厳密になる == 想定外の動作でエラーが出やすくなる == 問題箇所に気づきやすくなり、デバッグがしやすくなる`

ということです。
ただその分知識も必要になってくるので、はじめのうちはvarで書いて、慣れてきたらconst, letにするといいんではないでしょうか。

varをconstに置き換えると動かないのはちょっとわからなかったです。
よくあるのはグローバル空間でconstで宣言し合って競合するパターンなのですが、今回は即時関数(function(){})()の中にはいっていますし。

[ムノクラ]

動作を17で指定したら、下記のエラーになりました。

コード: 全て選択

rpg_managers.js:1949 ReferenceError: $gamevariables is not defined
    at Game_Actor.performVictory (MNKR_SelectWinAction.js:48)
    at rpg_objects.js:5172
    at Array.forEach (<anonymous>)
    at Game_Party.performVictory (rpg_objects.js:5171)
    at Function.BattleManager.processVictory (rpg_managers.js:2635)
    at Function.BattleManager.checkBattleEnd (rpg_managers.js:2617)
    at Function.BattleManager.updateEventMain (rpg_managers.js:2283)
    at Function.BattleManager.updateEvent (rpg_managers.js:2274)
    at Function.BattleManager.update (rpg_managers.js:2244)
    at Scene_Battle.updateBattleProcess (rpg_scenes.js:2265)
SceneManager.catchException @ rpg_managers.js:1949

どうも、これはconstをvarに置き換えた結果のエラーみたいです。

変更前のものに戻した動作確認は
17　戦闘不能
18　ツクールデフォルト
となりました。

なんか、少し変なので、もう少し落ち着いて、状況報告をしますね。

[ムノクラ]

変更前のものに戻した動作確認は
17　戦闘不能
18　ツクールデフォルト
となりました。

なんか、少し変なので、もう少し落ち着いて、状況報告をしますね。

大元のプラグインの構造にバグがありそうです。
一度、指定した終了時のモーションがリセットされていないため、次の戦闘で、最初がそのモーションになってしまっています。

エラーこそ出ませんが、これは想定外なのでバグです。
Sprite_Actor.prototype.refreshMotion の処理が終わってから、どこかでリセットする必要があるってことだと思うのですが…

[utako]

>「プラグインパラメータに配列として保存できるようにする」というのも、理解できていません…
> 申し訳ありませんが、少し踏み込んだ解説をいただければ幸いです。

リストの中身をプラグインユーザーが自由に編集できるようにするなら、プラグインパラメータにするのがいいかと思いました。

このような感じです。

MNKR_SelectWinAction.js

(2.01 KiB) ダウンロード数: 2 回

ataiが範囲外だった場合にコンソールにエラーを出すようにもしてみました。

元のプラグインや戦闘時の動作については詳しくないので、不具合に関してはお力になれそうにないです。。

[utako]

失礼。オブジェクト化した時にインデックスがズレたせいでモーション動作もズレてしまいましたね。
一つ↑のバージョンではズレは直っているはずです。

[Plasma Dark]

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

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

内部で 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） で発行された アクターのモーションを再描画してくれ 、というリクエストを検知した際に呼び出されるメソッドです。
これが再定義されてしまうと、戦闘中のモーションリフレッシュのタイミングで、再定義されたほうが呼び出されてしまい、アクターは勝利モーションの変更先として指定したつもりのモーションを取るようになってしまいます。