【コードレビュー】クレーンゲーム制作でハマりがちな落とし穴と改善ポイント

原文

もう全然ねできてないのに先のプログラム作っちゃダメもうすごい見た目の視認性が悪いもうハマりますこれは絶対ハマるこの動画ではソースコードのレビューをしてみたいと思います動画見てくれている方から作っているプログラムを見てほしいハードウェアもなんですけど依頼を受けて見てるんですけど作り方がみんな大体こういうふうに最初やるだろうなって形そのままできてるんでこの際なんでこのソースコードに対するそのコメントレビューをしてみたいと思います作っているものはクレーンゲームですね皆さんイメージするようなクレーンゲームでボタンが2つあって一つ目のボタンを押すとアームが上下前後か前後に動いてもう一つのボタンを押すと左右に動くその位置決めが決まったらアームですねそれが下に落ちて掴む動作をして戻して最初の位置に戻るっていうイメージするようなクレーンゲームですねそれを作ろうとしてますソースコードだけに専念して話をしますけど配線がスイッチさっき言った前後左右に動くスイッチあとそのモーターを動かすモータードライバーへの配線とかがあってそこら辺を定義してるんですけどコインストイントで今回定義してるんですけど普通、sharp define使うんですよ。sharp define pin 1 aが5みたいな感じにして、同じように全部sharp defineで2 aだったら6みたいな形にするんですね。で、これ何が違うかっていうと、const intで持つと、宣言した変数がメモリ上にずっと存在するんですよ。プログラムをプログラムをマイコンが使えるように機械語に翻訳するコンパイルっていうんですけどその時にフリープロセスっていう動作があってそこでシャープデファインで書かれたピン1 a今回で言うとピンの1 aっていうものがあった場合は後に置き換えるっていう処理をするんですねなので出来上がったプログラムの中にはPin 1 aっていうものはメモリ上には存在しないんですよ機械語に翻訳するときに5っていうものに置き換えられているだけなのでメモリの節約につながるのでこれは細かい話なんですけどやっといた方がいいと思いますなのでまず一つ目はコンストでピンは定義せずにデファインで定義しましょうって話ですねはい次ごめんなさいこれちょっと今更なんですけどArduinoで作ってますそのピンモードでいろんなピンの設定をしてやってるのがセットアップですね。ここはこれでいい。セットアップっていうのは基本変数の初期化とか、なんか設定を初期設定するっていうことで使うものなんで、これはいいです。ここからが長くなるんですけど、ここで前後のプログラムが書かれてます。次。またホワイル1で上と同じような感じのプログラムが動くんですけどこれが左右でごめんなさい説明してなかったんですけどそのボタンにはLEDがついていてそのLEDの設定とかをしているのがここですねその後アームの上げ下げ最後にアームを初期位置に戻すっていうプログラムまでで最後って感じですね。まずちょっと言いたいのが、まずこのプログラムを私が初めて見た時に、うまく動かないんですという話でもらって、このプログラム見た時に、ここまで見ると、アームの上下まではできてて、最後の原点戻りでつまずいてるんだっていうふうに思っちゃうわけですよ。何でかっていうとプログラム作る時っていきなり前後左右上下戻りみたいなそれ一気に全部書いて動かすんじゃなくてまずじゃあ前後の動きだけを作って問題なく動くようにしましょうでそれが終わったら左右に動くプログラムを追加しましょうこれができたらLEDの処理して問題ないこと確認したら上下の動きを確認してっていう風にやるんですけどなんとこのプログラムは全部の工程を作っているものの全部がうまく動いてないんですよまあ厳密に言うと最初だけはうまく動いてるっぽいんですけど左右は動かないまあledぐらいはなんとかなりますけど上下も動かない戻らないみたいなもう全然ねできてないのに先のプログラムを作っちゃダメなんですまず本当に小さく作って固めて組み合わせるんですよいきなり全部を作っちゃダメじゃあちょっともういろいろ言いたいんですけど、ループ関数っていうのはそもそもずっとぐるぐるぐるぐる回るんだから、回るループの中にホワイルループは基本入れないんですよ。なんでこうしたかっていうのもまあわかるんですけど、結局前後左右上下みたいなそのステップを踏んでいきたいという時に、よくやる方法としてはステータスみたいなのを持たせておいて切り替えるんですよ。例えばまずその左右っていうのを作ってこれを0としましょう同じように左右前後上下戻りみたいな作っておいてそれをそれぞれ0123みたいに吹っ飛ぶんですよね最初初期設定としてステータスっていう変数を持たせておいて最初スタートは左右ですみたいな風に宣言しておくんですよんでif文でもスイッチ文でもいいんですけど初めての人が分かりやすいようにif文で言っておきますとif startが左右だったら前後の処理をするそうでなくもしstartが前後だったらごめんなさい間違えちゃった最初がその前後として前後の処理を入れました左右の処理を入れますさらにそうでなくもしスカートが上下だったらLEDを忘れていたまあけどLEDはそんなに重要な問題ではないので前後左右の左右の処理の最後に入れるってことにしてでアームの上下だったらあーこれあれですねちょっとこれも言いたいんですけど、自作してほしいんです。インデント。Tabキーを押すとインデントするんで、Shiftを押しながらTabキーを押すと戻るんで、同じ階層は同じインデントにしてほしいんです。例えばこのif文だとこの直線上にいるじゃんみたいな次のそのif文の中はこの中のインデントなんですけどこれかこれが一段なぜかずれてるのでこれ下げないともうすごい見た目の視認性が悪いのでインデントはちゃんとするそんな感じでelse if thatが最後の戻りだったら最後のこの処理をこんな感じで最後のelse何もないときもしstatが何でもない場合は何もしないという感じで振り分けてやると前後左右上下戻りっていうあとそうでない時って大きくここで分けられるじゃないですかこれの中をループでぐるぐる回るのでステータスの変数を見てやりながらプログラムはその中だけにフォーカスして回すことができるんでそうなるとホワイルとかいらなくなるんですよ最初ステータスが前後だったらずっとぐるぐる回ってるんでホワイルを消し単純にさっき作ってたホワイルの中だけのやつにしてやればいいですこれを同じように左右のホワイルも取る取ってインデント1個上げる他も同じようにそれで中身を見ていきたいんですがLEDを光らせます消しますっていう処理が定期的に来るんですけどその間をどうやって待たせるかっていう時にディレイで待たせてるんですけどこのディレイも使っちゃダメなんでかっていうとこれはディレイっていうのはブロッキング処理って言って400ミリ秒間待ってる間は何のマイコンは処理できないんですね基本マイコンっていうか組み込みの考え方としてはいつでも何でもできる状態にしておくべきであってリレーで400ミリ秒何もボタンを押しても反応しないとか、リレーっていうのは極力使っちゃダメなんですよね。ではこれどうやってその400ミリ秒待たせるかっていう話なんですけど、ここで使うのがMillisですね。マイコンの電源が入ってからの動作時間を教えてくれる関数になってまして、変数を作らなきゃいけなくて、アンサインドロング待ち時間とかしておきますがこれを0ってしておきますよね最初ここLEDつけました400ミリ待ちますっていう時につけましたの瞬間に待ち時間にミリ図でその現在時刻を入れるもし今の時間がさっき設定した待ち時間ん?400ミリ秒より大きかったらLEDを消して待ち時間に今の時刻を入れるなのでもうちょっと変えますけど現在時刻が何かした時の時間から指定時間以上経ったらこれこれみたいな風にしてやるといいですまずはじめにちょっとごめんなさいまず待ち時間から400ミリ秒以上今の時刻で経ったらその現在時刻を待ち時間に入れますそれでもしLEDオンこれはLEDをつけるか消すかのプラグとして使いますけどLED点灯、消灯を管理するためのフラグLEDONっていうフラグが0だったらこれが点けるLEDONを1に切り替えるんですよねもしLEDONのフラグが0ではないつまり1なんですけどそうだったらLEDを消す消してLEDオンのプラグを0にリセットするっていう感じでさっきのLEDを点灯する400ミリ秒待つ消灯する400ミリ秒待つとかじゃなくて常にこう回るんですけど現在時刻を入れた待ち時間という変数に400ミリ秒を足された時刻以上であれば待ち時間という変数を現在時刻に更新してLEDを点灯するか消灯するかの処理をするのでこの時にマイコンは400ミリ秒待ってないんですよもうずっと自分は自分で時間を測っててループの中でひっきりなしに待ち時間たす400ミリ秒以上の数字に現在時刻になっているかっていうのをひっきりなしに見てるだけなので手は空いてるんですねなので滑らかな処理にはなりますスイッチそのボタンを押して動かしますアームを動かしますっていうところで、もともとスイッチをプルアップで設定しているので、そのスイッチを押されたら、デジタルの入力の結果は、もともと電源電圧が来てるのがスイッチを押さえることでグラウンドに落ちるんで、デジタル入力の結果は押すとゼロ。離すと1なのでイコールゼロつまり押してゼロになってるんだったらここからはちょっと作りが微妙でまたホワイルド分で回して押しているかつこれ端にあるリミットスイッチなんですけどリミットスイッチもう押されたらデジタル入力の結果がゼロになるっていうことっぽいのでボタンを押しているかつ端のスイッチを押してないだったらモーターを動かします端のスイッチを押したもしくは前後のボタンを離したらこのホワイルの条件がフォルス当てはまらなくなるのでこのホワイルを抜けてこれはモーターを止めるっていうことを書いてるんですけどモーターを止めてこのブレークはごめんなさいさっき取っていいよって言ったホワイル1にかかるブレークだったのでちょっとこれは消すとしてこんな形になっているとこれも押しているっていうことを押してデジタル入力の結果がゼロになった瞬間ホワイルループに入れるんじゃなくて押したらモーターは動かしちゃってそうでなければ止めるっていう形をまず基本として作っておいて押している中で、また変数を作らなきゃいけなくて、またそのフラグでbool、ボタン、pushedとかにしておきますか、0にして、そのボタンを押したら、まずボタン、pushedフラグを1にしておいて、離したっていう処理を確認するためには離したっていうのはボタンを押してないっていうことなんでここに飛ぶんですけどもともと押してなかったのか離したことによって押してない状態になったのかを検出するためにそのボタンプッシュドっていうプラグを持たせておいてもしその押してないっていう状態がボタンを押していた状態から押してない状態になったんだったらもうその前後の処理は終わりなんで次左右に移りたいんですこの移る時にステータスの変数をもともとずっと前後でぐるぐる回ってたやつを次左右に切り替えるんですねここの処理モーター止めるという処理はやってリミットスイッチを下押してないはさらにこの上の段階で見る必要があって、ifリミットスイッチを押されたかみたいなifがあって、その中にこれを入れてやればいいですね。はい、という感じで、じゃあこれ消します。そうすると、この前後の動きを振り返ると、最初ディレイで400ミリ回ってたやつをミリ図に変えたことでずっとマイコンは自分の時間と元々設定した時間以上経ってるかっていうのを見て手は空いてる状態になってますので何かしら処理をすることがいつでも可能な状態にできましたボタンが押されたかどうかの処理作りが微妙だったやつをボタンプッシュドっていうフラグを持たせたことでボタンを押したか押してないかでまず大きく2つに分けて押した時はモーターを動かしますでそれと同時にフラグの変数をセットしますボタンを押してないという状態がもともとずっと押してないのか押されてた状態から離したことによって押してない状態になったのかっていうことをボタンプッシュドっていうフラグで確認することができてもしその押してないという状態がボタンを離したことによって押してないっていうことになってるんであれば前後の処理は終わりなんでステータスを次のもともと前後だったやつを左右に切り替えましょうみたいな風にしてやればいいですあ、でごめんなさいちょっとこれあれだなボタンプッシュドはクリアした方がいいのでフラグもともとセットされてたボタンプッシュドのフラグを0にクリアって感じですねでこれと同じ感じで前後作ったやつを左右にも展開するんですよまずこの作り替えた処理が確実に動くってことを確認したらですね確認したらたらその左右の方向左右の関数にも展開させて今度左右も同じようにちゃんと動くかっていう確認するこれができたらじゃあ次アームの上下に移りましょうみたいな風に一気に全部作らずに段階を踏んでいかないともうハマりますこれ絶対ハマるので1個ずつ着実に動かしてそれを積み上げるっていう風にやらないとダメですねプランに言うとちょっとこんだけの行動なんでまだいいんですけど、できればあれですね、カンフーにまとめといた方が初めて見る人にとってはかなりありがたい。例えば、ボイドfunc前後ってやっといてそのfunc前後っていう関数の中にさっき作り直した処理をがっつり入れてやってで、このif…あ、ごめんなさい、ちょっと間違えたちょっとコピペするところ間違えたさっき作ったやつをがっつり関数の中に入れてやってこのfunc前後っていうのをステータスが前後だったら呼びますっていう風にしてやるとかなり見た目が分かりやすいですね前後だったらこの関数を処理するんだということでここだけを見ればいいっていう風になるんで初めて見る人にとってはすごく優しいですねすごくまとまりのいいプログラムになると思いますこんな感じで作ってもらえるとありがたいです動画はこれで終わります全然いいんですよこれは全然いいこういう風に見てくださいっていうのはすごくいいことでいきなりこういう風に書いてって言われても分かんないからとりあえず書いたやつを経験のある人が見て直すっていうことでプログラム書くっていう時のスキルを上げるのに役立つと思います全然これは恥ずかしいことじゃないしなんでこういう風に書くんだってなるのはそれは教わってないから当たり前なので気にする必要ないですただちょっと何回もやりとりしてるとちょっと疲れてきて言葉が荒くなってすいませんっていう感じで皆さんも私組み立てっていうサイトでハードもソフトも勉強できるような環境は作ってるんでなんか自分で作りたいなって思った時に動かないっていうのは往々にしてあるんですよ今回みたいにねあるんですけどそういうのは全部とりあえず見せてくださいって感じですねずっと悩んでるんじゃなくて経験のある人が見ればこれ微妙やなとか一瞬で分かるのでだからこそ前の動画でも言ったようにとにかく作って転職活動とかしたいっていう時にコード見せてとかになった時に見たらこういうので分かるんですよ作り方が一般の感じやなとかちょっとこれ経験した人の作ってる感じのプログラムだなってのはもう一瞬で見てわかるのでそういうのも組み立てでは面倒見たいと思ってるんでぜひ皆さんやってみてもらえるといいかなと思いますいろいろつまずくと思うんですけど経験のある人に見てもらえばもう一瞬で解決することも多いんで組み立てでそういうこともやってるっていうことで気が向いたら会員登録とか商品買って手で自分で動かしてみるなりしてみるといいのかなというふうに思いますということで以上でこの動画は終わりますありがとうございました