悪いソースコード - 独身エンジニアブログ

10年以上開発の現場でコードを書いたり読んだりしていると、悪いソースコードに出くわす機会も少なからずあります。

今回はPHPのサンプルコードを例に、悪いソースコードとその対策についてまとめました。

プログラミング初心者の方の参考になれば幸いです。

悪いソースコードとは？

そもそも悪いソースコードとはどういうソースコードなのか？

いくつも原因が考えられますが、下記のもので改善が可能なものを指すことが多いです。

処理に時間が掛かり負荷が高い書き方
バグにつながる書き方
コピペが多くコンポーネント化(関数化)されていない書き方
純粋に見づらい(分かりづらい)書き方

順番にサンプルソースコードを例に解説します。

処理に時間が掛かり負荷が高い書き方

一番分かりやすい例が、if elseの文です。

性別を判定する処理を例にします。(0: 不明 / 1: 男 / 2: 女 / 9: 適用不明)

<?php
// 悪いソースコードの例
if ($sex === 0) {
    echo "不明";
}
if ($sex === 1) {
    echo "男";
}
if ($sex === 2) {
    echo "女";
}
if ($sex === 9) {
    echo "適用不能";
}

プログラミング初心者の方が陥りやすい書き方です。

このソースコードの問題点は何度もifの判定を繰り返している点です。

$sexが1の場合、内部では下記のように処理されます。

$sexが0か判定 → $sexが1か判定("男"と表示) → $sexが2か判定 → $sexが9か判定

目的である性別の表示が終わっても、以降の判定を行っているため無駄な処理が存在して負荷が高くなっています。

<?php
// 良いソースコードの例
if ($sex === 0) {
    echo "不明";
} else if ($sex === 1) {
    echo "男";
} else if ($sex === 2) {
    echo "女";
} else if ($sex === 9) {
    echo "適用不能";
}

else ifを使ってあげることで、性別の表示以降の判定が行われないため、負荷を減らせます。

$sexが0か判定 → $sexが1か判定("男"と表示)

状態を判定する場合、基本的にはelseifを使うと覚えておけば問題ないと思います。

※どうでも良いですけど、変数名やカラム名にsexって付けるとレビュー時にちょっと恥ずかしいですよね…。

バグにつながる書き方

パターンとしては色々ありますが、最も多いのは型を意識していないためにバグにつながるケースです。

<?php
$str = 'hogehoge';
$find   = 'hoge';

$pos = strpos($str, $find);

if ($pos != false) {
    echo "OK";
} else {
    echo "NG";
}

// 結果
NG

strpos関数は、文字列が含まれていた場合にその位置を返す関数です。

この関数を利用して、$strに’hoge’という文字列が含まれているかどうかを判定し、含まれていたらOKと表示します。

一見良さそうですが、結果はNGとなりました。

今回は一番先頭にその文字列が含まれていたため、先頭を指し示す0がstrpos関数によって返されますがfalseも数値としては0なので、意図した挙動にはならなかったのです。

<?php
if (0 == false) {
    echo "一致";
}
// 結果
一致

数値としては一致しますが、型は当然違います。

<?php
echo gettype(0), "\n";
echo gettype(false);

// 結果
integer
boolean

これを回避するためには、型も判定に含める!==演算子を使う必要があります。

<?php
$str = 'hogehoge';
$find   = 'hoge';

$pos = strpos($str, $find);

if ($pos !== false) {
    echo "OK";
} else {
    echo "NG";
}

// 結果
OK

strposはよく利用されるため知っている方も多いと思いますが、他の関数になると戻り値を意識していないソースコードをよく見かけます。

※戻り値がnullに対してisset関数を使用したり、変数の存在を判定するためにempty関数を使用したり等。

型を意識するだけでも相当数のバグを減らすことができるので、関数の戻り値の型をチェックすることをおすすめします。

コピペが多くコンポーネント化されていない書き方

例えばユーザー情報を取得する下記のようなSELECT文の関数があった場合です。
※Laravelの書き方を例にしています。
※Laravel自体まだ触り始めたばかりなので、間違っていたらご指摘頂けると助かります。

<?php
public function get_users(): User {
    $users = User::get();
    return $users;
}

後から機能が追加され、nameでソートする必要が出てきたとします。

その場合に下記のような関数を別途追加してしまうパターンです。

// $sortには'desc'か'asc'を指定
public function get_users_sort_by_name($sort): User {
    $users = User::orderBy('name', $sort)->get();
    return $users;
}

一見すると良さそうですが、SELECTする際の条件を追加する必要があった場合はどうでしょう？

その場合、get_users関数もget_users_sort_by_name関数も両方修正する必要がでてきます。

関数が2つだけならまだ大丈夫ですが、3つ4つと同じような関数が増えてきた場合、全部に修正が必要になってきます。

これの恐ろしいのが修正が漏れてしまうケースが発生することです。

この修正漏れがバグを引き起こす原因になってしまいます。

そうならないためにも、条件(where文)が同じであれば下記のようにget_users関数に引数を追加してあげることでこの問題を回避できます。

// get_users()で呼び出せばソート無し
// get_users($sort)で呼び出せばソート有り
public function get_users(string $sort = NULL): User {
    if (is_null($sort)) {
        $users = User::get();
    } else {
        $users = User::orderBy('name', $sort)->get();
    }
    return $users;
}

こうすればwhereが複雑になったとしてもget_users関数内だけ修正すればOKなので、手間もバグも減らせます。

この例以外にも「あれ…なんか似てるような処理書いているな」と思ったら大抵の場合コンポーネント化できますので、面倒くさがらず処理をひとまとめにできないか模索してみてください。

純粋に見づらい(分かりづらい)書き方

見づらくなってしまう原因については、下記があげられます。

インデントがずれている
1つの関数の処理が長い(行数が多い)
文字数が多く改行がされていない
ifのネストが多い

このあたりについては分かりやすいので説明は割愛しますが、プログラミング歴が長い人でも陥りやすい書き方を紹介します。

明示的でない

この変数にはどういうデータが入っているのか？またはこの関数はどういう処理をするのか？というのが明示的でないのも悪い書き方になります。

実際に僕がやっていた悪いソースコードの例になります。

<?php

public function get_data(): array
{
	$data = array();
  $data['meats'] = array('cow', 'pig');
	$data['fruits'] = array('apple', 'cherry');
	$vegetables = 'potato';
	
	/*
	省略
	*/

  return $data;
}

この関数が例えば1000行以上になったと仮定します(1つの関数あたりの行数が長いのも問題ではありますが、ここでは説明のため…)

冒頭で$dataに’meats’と’fruits’というキーでデータを格納していますが、$vegetablesは格納していません。

この関数の処理を第三者の人がreturnまで読んだ際に「あれ、$vegetablesってこの$dataに含まれていたっけ？」となってしまい、処理をまた追いかける羽目になります。

そうならないためにも、returnで返す値を直前で格納するようにした方が読みやすくなります。

<?php

public function get_data(): array
{
  $meats = array('cow', 'pig');
	$fruits = array('apple', 'cherry');
	$vegetables = 'potato';
	
	/*
	省略
	*/
	
	// $vegetablesが入ってないことが明確
  $data = array();
	$data['meats'] = $meats;
	$data['fruits'] = $fruits;

  return $data;
}

関数の戻り値や三項演算子の多用

これは悪いソースコードとは言い難いので迷いましたが「読みづらい」という点では悪いソースコードといえなくもないので、載せておきます。

少し無理やりですが、下記みたいな書き方です。

※ちなみに僕もよくやりがちなので、戒めの意味も込めて載せています。

<?php

return $cnt > 0 ? (get_mode() === 1 ? get_status() : "error") : NULL;

カッコを付けていても分かりづらいですが、三項演算子のネストです。

熟練エンジニアの方や参考書にもこういった書き方で載っていますし、1行でまとまっているので美しいようにも見えます。

ですが分かりやすいかと言われれば間違いなく分かりづらい書き方です。

組織での開発は複数人で行うため、誰が見ても分かりやすい書き方がベターです。

<?php

$mode = get_mode();
$status = $mode === 1 ? get_status() : "error";
return $cnt > 0 ? $status : NULL;

エレガントな書き方より見やすい書き方の方が読みやすいので、バグも生まれにくくなります。

良いソースコードを書くには

書いたコードを1行ずつ理解して読み直すことを強くおすすめします。

関数の戻り値の型に合った判定をしているか？
負荷を掛ける処理になっていないか？
似たような処理をいくつも書いていないか？
変数名や関数名は適切か？
1行ずつ人に説明できるか？

上記のポイントを抑えながら読み直してリファクタリングをするのがおすすめです。

長くなりましたが、最後に良いソースコードを書く上でおすすめの書籍を紹介します。

最後に

悪いソースコードの例は挙げればキリがありませんが、よく見るパターンをまとめてみました。

まだ思いつく悪いソースコードのパターンはあるので、今度また余裕があった時にまとめてみます。

参考になりましたら幸いです。

最後まで読んで頂きありがとうございます。

独身エンジニアブログ

独身貴族を満喫しているアラフォーの雑記ブログ

「悪いソースコード」タグアーカイブ

悪いソースコードとはどういうソースコードか？