【完了】プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

アバター
ムノクラ
記事: 2011
登録日時: 2018年2月23日(金) 11:41
連絡を取る:

【完了】プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by ムノクラ » 2020年3月25日(水) 19:09

しぐれんさん、とんびさんにアドバイスいただいて作ったのですが、その後に機能を追加したので、余計なことをしていないか気になっています。
まだ、プラグインを作り始めて間もないので、コードレビューをお願いします。

SS01.png


※アイコン指定のパラメーターがstringなのは意図したもので、numberにしてしまうと、右クリックのアイコンビューアが使えなくなってしまうのを防ぐためです。
最後に編集したユーザー ムノクラ on 2020年3月25日(水) 23:47 [ 編集 2 回目 ]

---
JavaScriptの基本を学習せずにツクールのプラグインやスクリプトを使って横着してゲームを作ろうとしている人間です。
そのような者なので、適当な投稿をするかも知れません。
他の方の投稿を信用してください。
アバター
くろうど
記事: 259
登録日時: 2016年1月22日(金) 20:52
お住まい: 東京都
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by くろうど » 2020年3月25日(水) 20:42

こんばんは。
とりあえず、1点確認させてください。

36行目に、

コード: 全て選択

if (icon) {

となっていますが、0 が入った場合 false なので、

<EnemyIcon:0>

と指定された場合、「Default Icon」で指定した値(初期値16)が使われます。
想定している仕様通りでしょうか?
▼だいたいTwitterにいます。たぶん。
https://twitter.com/kuroudo119
アバター
トリアコンタン
記事: 2311
登録日時: 2015年11月10日(火) 21:13
お住まい: きのこ王国
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by トリアコンタン » 2020年3月25日(水) 21:12

こんにちは!

試しにレビューしてみました。
機能的にはおそらく正常に要件を満たしていると思われます。
なのでレビューに応じて実装を変更するかどうかはお任せします。

https://github.com/triacontane/RPGMakerMV/pull/40/commits/88d789e5f6f75d1fdcde43adc24b81fa0ccd14e2
最後に編集したユーザー トリアコンタン on 2020年3月25日(水) 22:08 [ 編集 2 回目 ]
プラグイン関連のトラブルが発生した際の切り分けと報告の方法です。
http://qiita.com/triacontane/items/2e227e5b5ce9503a2c30

[Blog] : http://triacontane.blogspot.jp/
[Twitter]: https://twitter.com/triacontane/
[GitHub] : https://github.com/triacontane/
アバター
ムノクラ
記事: 2011
登録日時: 2018年2月23日(金) 11:41
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by ムノクラ » 2020年3月25日(水) 21:14

くろうど さんが書きました:こんばんは。
とりあえず、1点確認させてください。

36行目に、

コード: 全て選択

if (icon) {

となっていますが、0 が入った場合 false なので、

<EnemyIcon:0>

と指定された場合、「Default Icon」で指定した値(初期値16)が使われます。
想定している仕様通りでしょうか?


ご指摘、ありがとうございます。
想定していないケースです。

これで良いのかどうか、考えてみます。
---
JavaScriptの基本を学習せずにツクールのプラグインやスクリプトを使って横着してゲームを作ろうとしている人間です。
そのような者なので、適当な投稿をするかも知れません。
他の方の投稿を信用してください。
アバター
ムノクラ
記事: 2011
登録日時: 2018年2月23日(金) 11:41
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by ムノクラ » 2020年3月25日(水) 21:40

トリアコンタン さんが書きました:こんにちは!

試しにレビューしてみました。
機能的にはおそらく正常に要件を満たしていると思われます。
なのでレビューに応じて実装を変更するかどうかはお任せします。

https://github.com/triacontane/RPGMakerMV/pull/40/commits/88d789e5f6f75d1fdcde43adc24b81fa0ccd14e2


レビューいただき、ありがとうございます。

大前提として、JavaScriptをほとんど理解していない(書き始めたのは…2週間前くらい?)で、他のプラグインを参考に「こうかな?」を繰り返して書いています。
そのレベルとご理解ください。

理解できる箇所と、できない箇所があります。

理解できる箇所
const を var に変更する。

理解できていない場所

40行 return index; を無効にすると、全てがデフォルト指定したアイコンになります。
おそらく、後述の重複コードを解決することと同時に対応する必要があると予想しますが、どういう分岐の書き方をするのかが分かっていません。
else if とか使うのでしょうか?

最後の処理にも return index; を入れていたのですが、削除しても動作したので、この方がベターなのかな?と思って書きました。

「コアスクリプトの既存処理と同じ実装になっている」のは理解できます。
ただ、その呼び出し方が分からないで、貼り付けました。
競合対策と呼ばれる処理(ここだと .call(index) ?)をする感じでしょうか?

とりあえず、分かる範囲で修正して、またアップしますので、覗いていただければ幸いです。
---
JavaScriptの基本を学習せずにツクールのプラグインやスクリプトを使って横着してゲームを作ろうとしている人間です。
そのような者なので、適当な投稿をするかも知れません。
他の方の投稿を信用してください。
アバター
トリアコンタン
記事: 2311
登録日時: 2015年11月10日(火) 21:13
お住まい: きのこ王国
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by トリアコンタン » 2020年3月25日(水) 22:08

こんにちは!

・return index; について

『return index;』というコードには二つの意味があります。
1. 関数の実行をここで終了する。
2. 関数の『戻り値』に「index」を指定する。

一般的なプログラミング言語の関数(メソッド)には『戻り値』という概念があります。
これは、関数の処理した結果を『呼び出しもと』に返却するときに指定する値です。
この例では「index」になります。

プラグインから『return index;』という行を除くと正常に動作しなくなります。
なぜなら、『1. 関数の実行をここで終了する。』というのが必要だからです。

一方、『2. 関数の戻り値に「index」を指定する。』は必要ありません。
関数によって戻り値が存在する関数と存在しない関数がありますが、今回の関数はコアスクリプトでは『戻り値が存在しない』関数でした。
よって『index』を戻り値として返却する必要はありません。

なので『return index』ではなく単に『return;』でOKです。

・コアスクリプトの既存処理を呼び出す方法について
これは決まり切った書き方があるので覚えてしまえばOKです。

コード: 全て選択

// 再定義前の関数を変数に入れてとっておく
var _Window_BattleEnemy_drawItem = Window_BattleEnemy.prototype.drawItem;
Window_BattleEnemy.prototype.drawItem = function (index) {
    // とっておいた関数を呼び出す。
    _Window_BattleEnemy_drawItem.apply(this, arguments);
};


レビューに応じてコードを変更した場合の一例です。
https://github.com/triacontane/RPGMakerMV/blob/feature/review_2/MNKR_EnemyIcon.js

変更後のコードでは、メモ欄から取得したアイコンを描画する場合と、プラグインパラメータから取得したアイコンを描画する処理をひとつにまとめた結果、『return』そのものもコードから消えました。
プラグイン関連のトラブルが発生した際の切り分けと報告の方法です。
http://qiita.com/triacontane/items/2e227e5b5ce9503a2c30

[Blog] : http://triacontane.blogspot.jp/
[Twitter]: https://twitter.com/triacontane/
[GitHub] : https://github.com/triacontane/
アバター
ムノクラ
記事: 2011
登録日時: 2018年2月23日(金) 11:41
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by ムノクラ » 2020年3月25日(水) 22:51

トリアコンタン さんが書きました:こんにちは!

・return index; について

『return index;』というコードには二つの意味があります。
1. 関数の実行をここで終了する。
2. 関数の『戻り値』に「index」を指定する。

一般的なプログラミング言語の関数(メソッド)には『戻り値』という概念があります。
これは、関数の処理した結果を『呼び出しもと』に返却するときに指定する値です。
この例では「index」になります。

プラグインから『return index;』という行を除くと正常に動作しなくなります。
なぜなら、『1. 関数の実行をここで終了する。』というのが必要だからです。

一方、『2. 関数の戻り値に「index」を指定する。』は必要ありません。
関数によって戻り値が存在する関数と存在しない関数がありますが、今回の関数はコアスクリプトでは『戻り値が存在しない』関数でした。
よって『index』を戻り値として返却する必要はありません。

なので『return index』ではなく単に『return;』でOKです。

・コアスクリプトの既存処理を呼び出す方法について
これは決まり切った書き方があるので覚えてしまえばOKです。

コード: 全て選択

// 再定義前の関数を変数に入れてとっておく
var _Window_BattleEnemy_drawItem = Window_BattleEnemy.prototype.drawItem;
Window_BattleEnemy.prototype.drawItem = function (index) {
    // とっておいた関数を呼び出す。
    _Window_BattleEnemy_drawItem.apply(this, arguments);
};


レビューに応じてコードを変更した場合の一例です。
https://github.com/triacontane/RPGMakerMV/blob/feature/review_2/MNKR_EnemyIcon.js

変更後のコードでは、メモ欄から取得したアイコンを描画する場合と、プラグインパラメータから取得したアイコンを描画する処理をひとつにまとめた結果、『return』そのものもコードから消えました。


return はイベントコマンドの「イベントの中断」みたいなものだと解釈しています。
indexを付けないと値を持っていってくれないと考えていたのですが、追加処理の部分が過剰だったために、余計な処理が必要になっていたのですね。
何となく分かりました。

最も勉強になった(と思っている)のは
var icon = parseInt(enemy.enemy().meta.EnemyIcon) || defaultIcon;
です。
なるほど、パラメーターの設定だけではなく、こうやって条件分岐を省略することも出来るんですね。
ただ、defaultIconを0にするとiconが無指定になりますが、defaultIconを無入力にすると、何かがある判定になるのは何故でしょうか?


.apply(this, arguments);
は先日、別の機会に教えていただいたばかりだったのですが、functionがindexだけを指定していたので、
.call(this);
で動くと思っていたのですが、エラーが出ていて理解できていませんでした。

if の分岐前に別の値を設定しているので、この処理でなければエラーになるのですね。
ようやくボンヤリと分かってきた気がします。


(これは入門者の入り口で、先に行くほど分からなくなる前の結構前の前兆…あと2年位やって、そこに行けるかどうかというところでしょうか)
---
JavaScriptの基本を学習せずにツクールのプラグインやスクリプトを使って横着してゲームを作ろうとしている人間です。
そのような者なので、適当な投稿をするかも知れません。
他の方の投稿を信用してください。
アバター
トリアコンタン
記事: 2311
登録日時: 2015年11月10日(火) 21:13
お住まい: きのこ王国
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by トリアコンタン » 2020年3月25日(水) 23:30

ムノクラ さんが書きました:最も勉強になった(と思っている)のは
var icon = parseInt(enemy.enemy().meta.EnemyIcon) || defaultIcon;
です。
なるほど、パラメーターの設定だけではなく、こうやって条件分岐を省略することも出来るんですね。
ただ、defaultIconを0にするとiconが無指定になりますが、defaultIconを無入力にすると、何かがある判定になるのは何故でしょうか?


defaultIconを無入力にした場合に何かがある判定になるのは、ここでデフォルト値「16」を設定しているからです。

コード: 全て選択

var defaultIcon = parseInt(parameters['Default Icon'] || 16);


『||』は、論理和と呼ばれる演算子で、上記の用例の場合は『parameters['Default Icon']』が有効(※)であれば、その値が返り、有効でない場合は16が返ります。
※有効とは、値が『空文字』『数値の0』『null』『undefined』など無効な値以外である場合をさします。

具体的にはここに記載されています。
https://www.javadrive.jp/javascript/ope/index13.html

上でくろうどさんが指摘しているのはこの点です。

パラメータに『0(文字列の0)』を指定した場合、『defaultIcon』には『数値の0』が指定されます。
なぜなら、文字列の0は『有効な値』だからです。
一方、パラメータに『空文字』を指定した場合、『defaultIcon』には『数値の16』が指定されます。
なぜなら、空文字は『無効な値』だからです。

正直、この理解はJavaScriptの仕様も絡んでくるので、すぐに理解するのは非常に難しいと思います。

ムノクラ さんが書きました:.apply(this, arguments);
は先日、別の機会に教えていただいたばかりだったのですが、functionがindexだけを指定していたので、
.call(this);
で動くと思っていたのですが、エラーが出ていて理解できていませんでした。


callでも動きますが、引数のindexの指定を忘れるとエラーになります。
callとapplyは似たような関数ですが、引数の指定方法が少し違います。

コード: 全て選択

_Window_BattleEnemy_drawItem.call(this, index);
プラグイン関連のトラブルが発生した際の切り分けと報告の方法です。
http://qiita.com/triacontane/items/2e227e5b5ce9503a2c30

[Blog] : http://triacontane.blogspot.jp/
[Twitter]: https://twitter.com/triacontane/
[GitHub] : https://github.com/triacontane/
アバター
ムノクラ
記事: 2011
登録日時: 2018年2月23日(金) 11:41
連絡を取る:

Re: プラグインのコードレビュー依頼:敵の名前の頭にアイコンを表示するプラグイン

投稿記事by ムノクラ » 2020年3月25日(水) 23:45

トリアコンタン さんが書きました:
ムノクラ さんが書きました:最も勉強になった(と思っている)のは
var icon = parseInt(enemy.enemy().meta.EnemyIcon) || defaultIcon;
です。
なるほど、パラメーターの設定だけではなく、こうやって条件分岐を省略することも出来るんですね。
ただ、defaultIconを0にするとiconが無指定になりますが、defaultIconを無入力にすると、何かがある判定になるのは何故でしょうか?


defaultIconを無入力にした場合に何かがある判定になるのは、ここでデフォルト値「16」を設定しているからです。

コード: 全て選択

var defaultIcon = parseInt(parameters['Default Icon'] || 16);


『||』は、論理和と呼ばれる演算子で、上記の用例の場合は『parameters['Default Icon']』が有効(※)であれば、その値が返り、有効でない場合は16が返ります。
※有効とは、値が『空文字』『数値の0』『null』『undefined』など無効な値以外である場合をさします。

具体的にはここに記載されています。
https://www.javadrive.jp/javascript/ope/index13.html

上でくろうどさんが指摘しているのはこの点です。

パラメータに『0(文字列の0)』を指定した場合、『defaultIcon』には『数値の0』が指定されます。
なぜなら、文字列の0は『有効な値』だからです。
一方、パラメータに『空文字』を指定した場合、『defaultIcon』には『数値の16』が指定されます。
なぜなら、空文字は『無効な値』だからです。

正直、この理解はJavaScriptの仕様も絡んでくるので、すぐに理解するのは非常に難しいと思います。

ムノクラ さんが書きました:.apply(this, arguments);
は先日、別の機会に教えていただいたばかりだったのですが、functionがindexだけを指定していたので、
.call(this);
で動くと思っていたのですが、エラーが出ていて理解できていませんでした。


callでも動きますが、引数のindexの指定を忘れるとエラーになります。
callとapplyは似たような関数ですが、引数の指定方法が少し違います。

コード: 全て選択

_Window_BattleEnemy_drawItem.call(this, index);


お示しいただいたページではありませんでしたが、『||』演算子については直ぐに検索して、微妙に理解した?という感じです。
先に満たした条件(or判定)を採用するという動作をするということですよね。

まだまだ、本当の意味での理解は先になるかと思いますが、今回はとても勉強になりました。
ありがとうございました。
---
JavaScriptの基本を学習せずにツクールのプラグインやスクリプトを使って横着してゲームを作ろうとしている人間です。
そのような者なので、適当な投稿をするかも知れません。
他の方の投稿を信用してください。

“MV:質問” へ戻る