Zenn
🔧

***ステッパーに対するコードレビュー / コードレビューの雰囲気を感じてみよう

2022/12/18に公開・約6,200字
Arduino
ポエム
#
コードレビュー
tech

はじめに

「図月つくる[1]」というYouTuberをご存じでしょうか。私は存じ上げています。
その図月つくるさんが「***ステッパー」というマウスの代わりに使う、運動が同時にできる入力デバイスを開発されました。
その入力デバイスの開発に当たり、組込みソフトウェアの開発を行われているのですが、そのソースコードが公開されています。
今回はそのソースコードを用いて、コードレビューの雰囲気を味わう会を行いたいと思います。

具体的には下記のような進行となります。

  1. デバイスの仕様説明
  2. デバイスのために実装されたコードの紹介
  3. 上記コードに対する私のコードレビューを公開

デバイスの仕様

本記事で取り扱うデバイスおよびソースコードの出典はこちらの動画[2]になります。

本記事で取り扱うデバイスは下記の手順で動作します。

  1. フットステッパーを踏み込む
  2. ステッパー下部に取り付けたフォトセンサーを反応させる
  3. センサーの信号をArduino[3]を用いてマウス入力信号に変換する
  4. PCへ左クリック入力をする

本デバイスの工夫された点としてステッパーの左右のペダルのうち、同じ側が連続で下げられた場合は入力として認めない点があります。
このような仕様とすることでステッパーを左右交互に上下させる必要が生じ、運動強度を上げることができます。

以上が本記事で取り扱うデバイスの仕様についてです。
今回コードレビューの対象とするのは、ステップ3で使用するArduinoのソースコードです。

今回のコードレビュー対象となるソースコード

//連続処理制御用
bool leftfoot = false;
bool rightfoot = false;

//マウス操作有効
#include <Mouse.h>
//ピンのセットアップ
void setup() {
  Serial.begin(9600);
  pinMode(7, INPUT);
  pinMode(8, INPUT);
  Mouse.begin();
}
void loop() {
  //左足ONかつ右足OFFかつ左足が連続しない時
  if (digitalRead(7) == LOW && digitalRead(8) == HIGH && !leftfoot) {
    rightfoot = false; //右足の連続制御を解除
    leftfoot = true;//左足連続制御ON
    Serial.println("左足で[LEFT]クリック有り"); //光電遮断
    Mouse.press(MOUSE_LEFT); //Mouseプレス
    delay(200);
    Mouse.release(MOUSE_LEFT); //Mouseリリース
    //左足OFFかつ右足ONかつ右足が連続しない時
  } else if (digitalRead(7) != LOW && digitalRead(8) != HIGH && !rightfoot) {
    rightfoot = true; //右足の連続制御ON
    leftfoot = false;//左足連続制御を解除
    Serial.println("右足で[LEFT]クリック有り"); //光電遮断
    Mouse.press(MOUSE_LEFT); //Mouseプレス
    delay(200);
    Mouse.release(MOUSE_LEFT); //Mouseリリース
    //両足OFF(両足ONは考慮しない)
  } else {
//    Serial.println("[LEFT]クリック無し");  //光電受光
  }
  delay(50);
}

コードレビュー

前提として、今回のレビュー対象のコードは仕様通りの動作をするため、最低限の品質をクリアしています。
そのため、レビューはソースコードのメンテナンス性向上を狙う内容となります。

※ 本レビューでは、フォトセンサーは信号を遮られたときに入力を検知する仕組みであることから、Arduinoコード内における信号 LOW が入力ON、信号 HIGH が入力OFFに対応付けられていると仮定しレビューをしています[4]。なお、仮にこれらが逆でも指摘内容の本質には影響を与えない認識です。

マジックナンバーを廃し定数とするべき

例えば、左側ステッパーに対応する入力のピン番号のIDである7を直接コード内で記述しています。
これらのような特定のIDを表す値や何らかの意図を持った値をマジックナンバーと呼びますが、マジックナンバーを直接記述するのではなく名前を付けそれを使用することが望ましいです。
その理由として下記が挙げられます。

  • マジックナンバーを記述するよりも名前が記述されている方が意図が伝わりやすいため。
  • 名前を定義する箇所のみの書き換えで、それを使用している個所全ての値を変更することができるようになるため。
  • 異なる意図ながら偶然値が一致したような値の取り違えを防ぐことができるため。

本件の場合、デバイス組み立ての段階で使用するピン番号は変化し得るため、それを予め想定し変更が容易となるような実装が望ましいです。

本件で値に名前を付ける方法としては #define[5] を使用することが望ましいです。
理由として、本件では実装時点で名前を付けるべき値を確定でき、かつその値を後から変更することがないためです。
このようなケースではコンパイル時に解決が行われる #define を使用することで実行時にコストがかからないようにできます。

実装の一例として、ピン番号に関連する箇所を抜粋すると下記のようなコードが考えられます。

~~~
#define LEFT_PIN_ID  7
#define RIGHT_PIN_ID 8
~~~
  pinMode(LEFT_PIN_ID,  INPUT);
  pinMode(RIGHT_PIN_ID, INPUT);
~~~
  if (digitalRead(LEFT_PIN_ID) == LOW && digitalRead(RIGHT_PIN_ID) == HIGH && !leftfoot) {
~~~
  } else if (digitalRead(LEFT_PIN_ID) != LOW && digitalRead(RIGHT_PIN_ID) != HIGH && !rightfoot) {
~~~

具体例はピン番号のみ記載しますが、delayの値なども同様に名前を付けることが望ましいです。
特に、左側ステップを踏み込んだ場合の処理と右側ステップを踏み込んだ場合の処理は対称的であるため、これらの処理の中のdelayの値は同じ名前で管理することが望ましいです。

左側ステップを下げた場合と右側ステップを下げた場合の処理が非対称

~~~
  //左足ONかつ右足OFFかつ左足が連続しない時
  if (digitalRead(7) == LOW && digitalRead(8) == HIGH && !leftfoot) {
~~~
    //左足OFFかつ右足ONかつ右足が連続しない時
  } else if (digitalRead(7) != LOW && digitalRead(8) != HIGH && !rightfoot) {
~~~

上記抜粋個所にて、左側ステッパーが下がった場合の処理では == で比較されていますが、右側ステッパーが下がった場合の処理では != で比較されています。
これらの比較演算子が異なるのは、何か特別な意図のもとでしょうか。

一般に、本件のようにラベルと比較する場合には関係するラベルの総数を知らなくてもよい実装が望ましいです。
現在のArduinoの仕様では、関係するラベルが HIGH と LOW の2種類しか存在せず not LOW == HIGH の関係が偶然成り立っていますが、この関係はラベルが3種類以上の場合に破綻します。
そのため、Arduinoのバージョンアップ等で例えば MIDDLE のようなラベルが追加されることを想定したコードであることが望ましいです。
これらの観点から該当のコードを確認すると、 MIDDLE ラベルが追加された場合に左側ステッパーの処理と右側ステッパーの処理の意味が変わってしまい対称性が失われるため、望ましくないと考えます。

本件の場合、 MIDDLE ラベルが追加されることを想定した実装方針の一例として LOW のみONでそれ以外はOFFが挙げられます。
具体的には下記のようなコードが考えられます。

~~~
  //左足ONかつ右足OFFかつ左足が連続しない時
  if (digitalRead(7) == LOW && digitalRead(8) != LOW && !leftfoot) {
~~~
    //左足OFFかつ右足ONかつ右足が連続しない時
  } else if (digitalRead(7) != LOW && digitalRead(8) == LOW && !rightfoot) {
~~~

より可読性を上げるため、ONの側、OFFの側の順に並べ替えるのもよいです。

~~~
  //左足ONかつ右足OFFかつ左足が連続しない時
  if (digitalRead(7) == LOW && digitalRead(8) != LOW && !leftfoot) {
~~~
    //右足ONかつ左足OFFかつ右足が連続しない時
  } else if (digitalRead(8) == LOW && digitalRead(7) != LOW && !rightfoot) {
~~~

あえて指摘しなかった点

  • コメントの字下げやスペースが不揃い
    • それはコードフォーマッタの仕事
  • 入力のピン番号と連続チェック変数は、構造体にしてセットで管理すべき
    • もう少し大規模なら指摘するが、この規模なら十分見通せるのでオーバースペックと判断
    • ところで、Arduino公式ドキュメントに構造体(struct)の記述がないのはなぜだろう
  • 左側ステップを下げた時の処理と右側ステップを下げた時の処理を共通化すべき
    • これも今の規模ならオーバースペックと判断
  • 左側ステップが下がったかのチェックと右側ステップが下がったかのチェックのそれぞれで入力のピン情報を取得しているが、これを1度だけ取得するようにする
    • 今の規模なら誤差なのであえて指摘をする必要はない
    • 入力を取得する箇所が増えIO負荷が無視できなくなりそうな場合は指摘する可能性あり

結論

今回取り上げたのはかなり短いコードですが、それでも重要な示唆を与える指摘ができたように思います。
いずれの指摘も今回のソースコードを動作させるためには対応せずともよい指摘ではありますが、このコードを今後メンテナンスすることを視野に入れる場合には考慮することが望ましいと考えます。

また、今回は通常コードレビューでは触れない、あえて指摘しないと決めた点とその理由についても述べました。
コードレビューは現在のプロダクトの規模に合わせることでよりよい体験とできることが伝われば幸いです。

最後に。

  • コードレビューでソースコードの品質を向上していこう
  • コードレビューでは愛を持って全力で殴り合おう

謝辞

本記事を作成するに当たり、下記の方々が作成された情報を参照いたしました(敬称略)
謹んで御礼を申し上げます。

  • 図月つくる, ライヴラリ所属, 株式会社ゆにクリエイト
  • 餅月ひまり, ライヴラリ所属, 株式会社ゆにクリエイト

本記事を作成するに当たり、下記の製品について取り扱いました。
謹んで御礼を申し上げます。

  • Arduino, Arduino Foundation および Arduino Holding
脚注

  1. 図月つくるさんのTwitterアカウント YouTubeチャンネル ↩︎

  2. ソースコードの出典動画 アダルト方面のセンシティブなコンテンツを含むため、苦手な方はご注意ください。 ↩︎

  3. Arduinoの公式ページ ↩︎

  4. LOW / HIGH ↩︎

  5. #define ↩︎

Discussion

ログインするとコメントできます