【完了】プラグインのレビュー依頼：敵グループのランダム化

[Plasma Dark]

全ての敵が出なくなり、自動勝利となります

conditionの値を初期化していないように見えます。
そのため、敵グループの設定を全く行わずに戦闘処理が進み、敵が誰もいないので自動勝利します。

敵グループの設定は、一度は必ず通ってほしい処理なので、do whileで書いてしまったほうが良いかもしれませんね。

[ムノクラ]

全ての敵が出なくなり、自動勝利となります

conditionの値を初期化していないように見えます。
そのため、敵グループの設定を全く行わずに戦闘処理が進み、敵が誰もいないので自動勝利します。

敵グループの設定は、一度は必ず通ってほしい処理なので、do whileで書いてしまったほうが良いかもしれませんね。

分かりました！
また、スコープの問題ですね。

重複しているかなと思って弄った箇所が裏目に出ていたようですが、今回の変更で動くようになりました。

冗長なコードだとは思いますが、これで動作は問題なくなったと思います。

[Plasma Dark]

こうなると、github（あるいはgitだけでも）の利用を強く推奨したくなりますね。
conditionを宣言している行での初期化がいつの間にかなくなっているというのは、ファイル名によるバージョン管理が失敗していることの証左です。

githubでは以下のように、コミットごとの差分を確認できます。

https://github.com/elleonard/RPGtkoolMV-Plugins/commits/master/plugins/DarkPlasma_TextLog.js

今すぐに始められるものでもないかもしれませんが、バージョン管理ツールとして極めて強力であり、今回のcondition未初期化のような本質的でないバグを防げる（すなわち、レビューの効率も良くなる）ということはご理解いただけるものと思います。

[Plasma Dark]

動作に関して言えば問題ないと思います。

[ムノクラ]

動作に関して言えば問題ないと思います。

ファクタリングが必要ですよね。

冗長なコードなのは自覚しているので、この数日で教えていただいた || を使ってみました。
これで1つ重複している箇所が省略できたと思います。

あとは、randomEnemyIdとenemyIdを上手く統合できれば一気に削れるような気がしたのですが、関数に分けたので、上手いこと統合（もしくは簡略化）することが、今の自分には出来ませんでした。

一箇所だけ、省略できたものを出します。

ここは「これはこう書くべき」というのを拝見して、比較しないと学習するのは難しいと感じています。


GitHubは、まだ夢の世界です。
少しずつ挑戦していますが…

[Plasma Dark]

randomEnemyIdとenemyIdを上手く統合できれば一気に削れるような気がしたのですが

実は、短いコードが良いコードである、というのにも限度があります。
多少冗長でも意味が伝わりやすいコードを書いたほうが良いです。

そういう意味ではひとまず十分なコードだと思います。

指摘する部分があるとすれば、以下の通りです。

- conditionが使われるのはsetup関数の中だけなので、setup関数の先頭で宣言、初期化するとスコープを限定できて良い。（というより、今のままだと2回目以降の戦闘で自動勝利しそう）
- 36行目のrandomEnemyIdの初期化は無意味なので、直接 var randomEnemyId = selectEnemyId($dataEnemies[member.enemyId]) と書いて良い。今の記述では、なぜか空配列で初期化されており、後で扱う値と型が違うため行儀が悪い。（しぐれんさんのご指摘にもありますが）
- 66行目のrandomEnemyIdは代入する必要がなく、直接返してしまっても良い。 return Number(pool[Math.randomInt(pool.length)]);

[ムノクラ]

randomEnemyIdとenemyIdを上手く統合できれば一気に削れるような気がしたのですが

実は、短いコードが良いコードである、というのにも限度があります。
多少冗長でも意味が伝わりやすいコードを書いたほうが良いです。

そういう意味ではひとまず十分なコードだと思います。

指摘する部分があるとすれば、以下の通りです。

- conditionが使われるのはsetup関数の中だけなので、setup関数の先頭で宣言、初期化するとスコープを限定できて良い。（というより、今のままだと2回目以降の戦闘で自動勝利しそう）
- 36行目のrandomEnemyIdの初期化は無意味なので、直接 var randomEnemyId = selectEnemyId($dataEnemies[member.enemyId]) と書いて良い。今の記述では、なぜか空配列で初期化されており、後で扱う値と型が違うため行儀が悪い。（しぐれんさんのご指摘にもありますが）
- 66行目のrandomEnemyIdは代入する必要がなく、直接返してしまっても良い。 return Number(pool[Math.randomInt(pool.length)]);

ご指摘いただいた箇所を修正し、動作確認できました。

1.conditionの宣言位置は変更前でも自動勝利しませんでしたが、理論的にはこちらのほうが正しそうというのが理解できた（つもり）なので、変更しました。
2.空配列で初期化していたのは、参考にしていたコードがそう書いていたからそのまま真似しました。
意味がないというのを直ぐに理解できませんでした。
削除して動作したのを確認し、必要ないことを理解しました。
3.なるほど、先程nullを返す時も、直接returnで返していたので、同じ理屈ですね。

とても勉強になりました。

ありがとうございました！

[Plasma Dark]

ifによる分岐を減らしたければ、以下のように書くことは可能です。

コード: 全て選択

var randomEnemyId = selectEnemyId($dataEnemies[member.enemyId]);
var enemy = new Game_Enemy(randomEnemyId || member.enemyId, member.x, member.y);
if (randomEnemyId === 0 || member.hidden) {
  enemy.hide();
} else {
  condition = false;
}
this._enemies.push(enemy);

Game_Enemyに渡すべき敵IDと、隠すために使うランダム選択IDが別の存在なので、コードは短くなりますがちょっとわかりにくいかもしれません。

[ムノクラ]

ifによる分岐を減らしたければ、以下のように書くことは可能です。

コード: 全て選択

var randomEnemyId = selectEnemyId($dataEnemies[member.enemyId]);
var enemy = new Game_Enemy(randomEnemyId || member.enemyId, member.x, member.y);
if (randomEnemyId === 0 || member.hidden) {
  enemy.hide();
} else {
  condition = false;
}
this._enemies.push(enemy);

Game_Enemyに渡すべき敵IDと、隠すために使うランダム選択IDが別の存在なので、コードは短くなりますがちょっとわかりにくいかもしれません。

まだ、直感的に理解できるほどではないですが、処理の意味は理解できました。
早速、反映したコードを実行したところ動作確認できました。
凄く短くなりますね…

[ムノクラ]

そうそう、ご報告が漏れていました。
しぐれんさんに、敵が隠れている処理を提案された時に「全体攻撃のスキルで、単体アニメーションを再生すると隠れている敵にも再生されてしまうという問題を見たことがある」というやりとりをしました。

現状の1.6.2では、この問題は確認されなかったので、コアのバージョンアップで解消されたのか、違うプラグインでのご動作だったのかもしれません。

よって、このプラグインでは、アニメーションの問題は確認されませんでした。