🔧

ラーメンオーダー自動生成装置に対するコードレビュー / コードレビューの雰囲気再び

2023/01/20に公開・約7,900字

Arduino

ポエム

コードレビュー

tech

本記事では非専業プログラマと思われる方の書いたコードに対するコードレビューを題材としています。
- コードの品質を咎める意図は一切ありません。
- プログラム作成者のコーディング能力について評価する意図は一切ありません。
- コードレビューの内容は真剣です。
- レビュー内容はやや過剰めに丁寧にしています。
本記事はプログラム作成者からの転載の許可を頂かずに記述しています。
- 本記事で取り扱うコードは、後述の動画コンテンツ内で取り上げられたものを写経したものです。
- 本記事のソースコードについて、私は二次利用者ですので、利用許諾の判断はできかねます。三次利用は控えるようにしてください。
- プログラム作成者からの申し出があった場合、速やかに記事の修正・公開停止などの対応を行います。
本記事は私の独断によって作成されています。
- 本記事の一切について、プログラム作成者への問い合わせをしないようお願いします。
- 本記事は公開プルリクエストではありませんので、ご理解をお願いします。

はじめに

「図月つくる^[1]」というYouTuberをご存じでしょうか。私は存じ上げています。
その図月つくるさんが「ラーメンオーダー自動生成装置」というラーメン二郎^[2]のオーダー（通称「ニンニク入れますか？」コール）をランダム生成する装置を開発されました。
その装置の開発に当たり、組込みソフトウェアの開発を行われているのですが、そのソースコードが公開されています。
今回はそのソースコードを用いて、コードレビューの雰囲気を味わう会第2回^[3]を行いたいと思います。

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

装置の仕様説明
装置のために実装されたコードの紹介
上記コードに対する私のコードレビューを公開

装置の仕様

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

本記事で取り扱う装置は下記の手順で動作します。

スイッチを押す
スイッチの入力を受け、Arduino^[5]を用いて下記の手順で5つのモータを制御する
1. すべてのモータを起動しロールを回転させる
2. 上から順番に下記の手順でモータを制御する
  1. ランダムで2～5秒待つ
  2. 対象のモータを停止させロールを停止する

以上が本記事で取り扱う装置の仕様です。
今回コードレビューの対象とするのは、ステップ2で使用するArduinoのソースコードです。

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

//スイッチ指定
const int button = 2;
//モータドライバ1-1
const int IN1 = 3;
const int IN2 = 4;
//モータドライバ1-2
const int IN3 = 5;
const int IN4 = 6;
//モータドライバ2-1
const int IN5 = 7;
const int IN6 = 8;
//モータドライバ2-2
const int IN7 = 9;
const int IN8 = 10;
//モータドライバ3-1
const int IN9 = 11;
const int IN10 = 12;

void setup() {
  // put your setup code here, to run once:
  pinMode(button, INPUT_PULLUP);
  pinMode(IN1, OUTPUT);
  pinMode(IN2, OUTPUT);
  pinMode(IN3, OUTPUT);
  pinMode(IN4, OUTPUT);
  pinMode(IN5, OUTPUT);
  pinMode(IN6, OUTPUT);
  pinMode(IN7, OUTPUT);
  pinMode(IN8, OUTPUT);
  pinMode(IN9, OUTPUT);
  pinMode(IN10, OUTPUT);
  Serial.begin(9600);
}

void loop() {
  // put your main code here, to run repeatedly:
  boolean value;
  value = digitalRead(button);
  Serial.println(value);
  //ボタンを押した時、全モータ動作
  if (value == 0) {
    digitalWrite(IN1, HIGH);
    digitalWrite(IN2, LOW);
    digitalWrite(IN3, HIGH);
    digitalWrite(IN4, LOW);
    digitalWrite(IN5, HIGH);
    digitalWrite(IN6, LOW);
    digitalWrite(IN7, HIGH);
    digitalWrite(IN8, LOW);
    digitalWrite(IN9, HIGH);
    digitalWrite(IN10, LOW);
    //2~5秒後に順にモータ停止
    delay(random(2000, 5000));
    digitalWrite(IN1, LOW);
    digitalWrite(IN2, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN3, LOW);
    digitalWrite(IN4, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN5, LOW);
    digitalWrite(IN6, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN7, LOW);
    digitalWrite(IN8, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN9, LOW);
    digitalWrite(IN10, LOW);
    delay(random(2000, 5000));
  }
  delay(100);
}

コードレビュー

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

繰り返し記述されている処理をループにすべき

本件のコードが将来的にどのように拡張されるか、つまりこの装置がどのように発展し得るかを考えた場合、真っ先に想起されるのはモータの数の増減です。
そのため、本件のコードの修正方針も、モータの数の増減に関する対応の容易性を実現するものとなります。

これを実現する方法にはいくつかの手法が考えられますが、最も手軽な実現方法はピン番号を配列で管理し、処理をループで記述する方法です。

    //2~5秒後に順にモータ停止
    delay(random(2000, 5000));
    digitalWrite(IN1, LOW);
    digitalWrite(IN2, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN3, LOW);
    digitalWrite(IN4, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN5, LOW);
    digitalWrite(IN6, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN7, LOW);
    digitalWrite(IN8, LOW);
    delay(random(2000, 5000));
    digitalWrite(IN9, LOW);
    digitalWrite(IN10, LOW);

例えば、上記のモータを止める処理ですが、ピン番号違いで同じ処理を5回繰り返しています。
このようなケースでは配列(array^[6])とループ(for^[7])を組み合わせると簡潔に記述できます。

    //2~5秒後に順にモータ停止
    for(int i = 0; i < MD_NUMS; i++) {
      delay(random(2000, 5000));
      digitalWrite(MDS[i][0], LOW);
      digitalWrite(MDS[i][1], LOW);
    }

上記のコードで、MD_NUMSはモータの総数を表す定数、MDSはモータのピンのペアを格納した配列です。
例えばMDS[0][0]は1番目のモータの奇数番ピンを表し、MDS[2][1]は3番目のモータの偶数番ピンを表します。

MD_NUMSおよびMDS配列は、例えば下記のように定義をします。

//スイッチ指定
const int button = 2;
//モータドライバ1-1
const int IN1 = 3;
const int IN2 = 4;
//モータドライバ1-2
const int IN3 = 5;
const int IN4 = 6;
//モータドライバ2-1
const int IN5 = 7;
const int IN6 = 8;
//モータドライバ2-2
const int IN7 = 9;
const int IN8 = 10;
//モータドライバ3-1
const int IN9 = 11;
const int IN10 = 12;

//モータドライバの総数
const int MD_NUMS = 5;
//モータドライバのピン番号ペアの配列
const int MDS[MD_NUMS][2] = {{IN1, IN2}
                            ,{IN3, IN4}
			    ,{IN5, IN6}
			    ,{IN7, IN8}
			    ,{IN9, IN10}
			    };

配列についての詳細

配列とは通し番号(index)が振られた一連の変数です。
例えば、int a[3];と宣言した場合、下記のような3つの箱を持つ配列が生成されます。

0	1	2
0	0	0

Arduinoの配列の通し番号は0から始まるため、1番目を参照する場合は0、2番目を参照する場合は1、3番目を参照する場合は2、のように指定します。

int a[3];
a[0] = 1;
a[1] = 2;
a[2] = 3;

この処理の結果、下記のようになります。

0	1	2
1	2	3

値を取り出すこともできます。

int v = a[2][0]; // v = 3となる

配列の宣言と同時に値をセットすることもできます。

int a[3] = {1, 2, 3};

配列の通し番号には変数が使用できるため、ループと組み合わせることで便利に通し番号を指定できます。

for (int i = 0; i < 3; i++) {
  Serial.println(a[i]);
}

また、配列の箱の個数指定には定数を用いることもできます。
下記のコードでは、3つの箱を持つ配列が生成されます。

const int SIZE = 3;
int a[SIZE];

さらに、配列は入れ子にすることができます(多次元配列と呼びます)
特に2重の入れ子になった配列を二次元配列と呼びます。
例えば、int a[3][2];と宣言した場合、下記のような配列が生成されます。

		0	1	2
0		0	0	0
1		0	0	0

二次元配列も同様に通し番号は0から始まります。

int a[3][2];
a[0][0] = 1;
a[0][1] = 2;
a[1][0] = 3;
a[1][1] = 4;
a[2][0] = 5;
a[2][1] = 6;

この処理の結果、下記のようになります。

		0	1	2
0		1	3	5
1		2	4	6

これらのデータに参照すると下記のようになります。
2つの通し番号を指定しますが、ちょうど上記のテーブルにおける横の通し番号と縦の通し番号に相当します。

int u = a[0][1]; // u = 2となる
int v = a[2][0]; // v = 5となる

2次元配列でも宣言と同時に値をセットすることもできます。

int a[3][2] = {{1, 2}
              ,{3, 4}
	      ,{5, 6}
	      };

2次元配列でも通し番号に変数を使用できます。

for (int i = 0; i < 3; i++) {
  for (int j = 0; j < 2; j++) {
    Serial.println(a[i][j]);
  }
}

本レビューで提案したコードは、2次元配列を使用すること、箱の個数を定数で宣言すること、宣言と同時に値をセットすることの合わせ技となっています。

前述のように実装することで、例えばモータの数が1つ増えた際に下記の2ヶ所の変更で対応することが可能となります。

MD_NUMS を6に変更する
MDS 配列の末尾に対応するピン番号のペアを追加する

上記に加え、ソースの記述例は示しませんが、下記の個所でも同様の変更を行うことでモータ数の変更に強いコードにすることが可能です。

setup 関数内のピンの初期化処理
loop 関数内のモータを回す処理

あえて指摘しなかった点

命名について
- 各変数、定数の命名には議論の余地がある
- コード量や文化を考慮すると、積極的に議論した場合のメリットが薄いと感じたため割愛
モータ起動/停止処理の関数化
- 意図がより読み取りやすくなる
- 今回の指摘点を反映したあとのコードでは、関数化をしなくても可読性は悪くないと判断し保留
- 今後の拡張方針によっては取り組むべき課題になり得る
その他軽微な指摘
- button定数もIN1などと統一するため大文字での記述が望ましい
- digitalRead(button);の戻り値の比較時にマジックナンバーを使用しているため、定数に置き換える
  - pinMode(button, INPUT_PULLUP);と指定しているため^[8]スイッチONはLOWであり、これと比較することが望ましい
- delay値などのマジックナンバーの定数化

結論

今回コードレビュー体験会第2回を実施しましたが、レビューを行う中で驚きの連続でした。
第1回での指摘内容が解消されており、本プログラム作成者の技術力向上を強く感じ取れるものでした。
それ故に、さらに良いコードにするためにはどのような方針でレビューをすればよいかを考えるのが難しくもあり、楽しくもあり、やりがいもありました。

今回のレビュー内容を通じて、より簡潔な実装のためにはどうすればよいのか、という観点の大切さを持ち帰って頂ければ幸いです。

最後に。

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

謝辞

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

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

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

Arduino, Arduino Foundation および Arduino Holding

本記事を作成するに当たり、下記の方々にご助言、ご支援を頂きました。
謹んで御礼を申し上げます。

助手くん, 図月つくるさんのファンの皆様(個別のお名前は差し控えさせて頂きます)

脚注

Discussion

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