Go to Qiita Advent Calendar 2024 Top
LoginSignup
7
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

Qiitanがほしい人の一人アドカレAdvent Calendar 2024

Day 1
@nikawamikan(Mikan Nikawa)inTeam shahu

クソコードから学ぶ 良いコードとは？

Posted at

読み飛ばしてください

どうも、限界派遣SESです。
今回どうしてもQiitanのぬいぐるみが欲しくてアドベントカレンダーを一人で日記的に25記事書こうとする哀れな男です。

そんなわけでアドベントカレンダー１日目ですが、最低限記事としての体裁を保たなくてはなりません。
そこで、最小限みなさまのお役に立てるようにナレッジ的な何かを提供できればとクソコードと良いコードついて書いていこうと思います。

クソコードとは？

読んで字の通りクソなコードです。
私もよく量産します。

例えば、データベースからユーザー名一覧を取得するような関数があったとします。

// あんまりよくない例

// ユーザー名一覧をDBから取得するための関数
List<string> GetUserNames()
{
    var connectionString = "...";
    var sql = "SELECT user_name FROM hoge";
    var result = new List<string>();
    
    using (var connection = new SqlConnection(connectionString))
    {
        connection.Open();
        using (var command = new SqlCommand(sql, connection))
        {
            using (var reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    result.Add(reader.GetString(0));
                }
            }
        }
    }
    return result;
}

別に大きく間違っているわけではないですね。少しクソなだけなコードです。

このコードの問題点をいくつかあげようとするなら

  • 関数名が明示的でない
  • 再利用性がない

という問題があります。

関数名が明示的でない

まずGetというワードは多くのプログラマ1にとって軽量アクセサである認識があります。

なので、以下のようなコードを書かれる危険性があるわけです。

static void Main(string[] args)
{
    // ユーザーの組み合わせを網羅する
    var pear = new List<(string, string)>(GetUserNames().Count); // DB全検索
    foreach (var user in GetUserNames()) // DB全検索
    {
        foreach (var user2 in GetUserNames()) // DB全検索
        {
            // 同一ユーザーでなければ追加
            if (user != user2)
            {
                pear.Add((user, user2));
            }
        }
    }
}

まあ、これはこれでクソコードですが、Getというニュアンスからはこのように読んでも問題はない感覚を得ます。

より良い名称をつけるには？

そこで最近読んで「それはそうだろう」と思った書籍でお馴染みリーダブルコードには以下のようにGetをつけないほうが良い理由を明示しています。

「名前に情報を詰め込む」には、明確な単語を選ばなければならない。「空虚な」語は避けるべきだ。
例えば、「get」はあまり明確な単語ではない。
def GetPage(url):
...
「get」という単語からは何も伝わってこない。このメソッドはページをどこから取ってくるのだろうか？ローカルキャッシュから？データベースから？インターネットから？
インターネットから取ってくるのであれば、FetchPage() や DownloadPage() のほうが明確だ。

つまり、Getという名称からはどこから取ってくるのか？どのように取ってくるのか？という重要な要素が抜けています。
また、引用にある通り個人的にもデータベースからデータを取得するという処理ではFetchを使うことを推奨したいと思います。
これはjavascriptでも特定のURLからデータを取得する場合にfetch関数を使うこととも直感的に一致します。

どのような名称をつけるかは以下の記事がとても参考になると思います。

また、筆者は英語が非常に苦手であり、変数名や関数名をつけることに毎回の苦労しています。
OUTPUT_JOTI_OUTPUT2のような途方もなく理解不能な変数名を想起するよりもCodicのような変数名をつける事に特化したサイトや、ChatGPTに聞いてみるべきです。

再利用性がない

私はカレーを作ってほしいとします。

そんなわけで私は以下のような指示を与えます。

  1. スーパーで以下のものを購入してください
    - カレーのルー: １箱
    - にんじん: 3本
    - じゃがいも: 3個
    - ぶたにく: 300g
  2. にんじんは皮を剥いてから2cm程度のサイズになるように切断してください
  3. じゃがいもは皮を剥いてから3cm程度のサイズになるように切断してください
  4. ぶたにくは3cm程度のサイズになるように切断してください
  5. 以下の要件に一致する鍋を用意してください
    - 4L程度の容量
    - IT調理が可能
  6. 鍋をITクッキングヒーターに置き電源を入れ油を大さじ3程度いれてください
  7. 十分に温まるまで(200℃程度)待ってください
  8. 切断したにんじんを鍋にいれてください
  9. 切断したじゃがいもを鍋にいれてください
  10. 切断したぶたにくを鍋にいれてください
  11. どうせここまで読む人はいないだろう
  12. ぶたにくに焼きめがついたら水1000ccを追加してください
  13. よく煮込まれたらカレーのルーを投入してください
  14. カレーのルーが溶けて粘度がいい感じであれば完成です

実際に古いソースコードを読んでいると、これに近しい命令を実装していることが多々あります。（筆者だけかもしれないが）
そもそも設計書の時点でここまで書いているものもあるだろうが、それはクソ設計書の話なのでここでは触れないことにします。

そもそも人間と会話していてこのような指示を与えるだろうか？もっと抽象的に指示すればいいのではないだろうか？

私は以下のように命令を訂正しようと思う

「カレーを作って」

これが理想であるし、プログラムもこのように書くべきだと考えています。
なぜなら作ってもらう側としてはカレーが美味しければこのレシピ通りである必要もないからです。

では先程のコードに戻って考えてみましょう。

// ユーザー名一覧をDBから取得するための関数
List<string> FetchUserNames() // 関数名はFetchを使うことにした
{
    // 接続先を定義
    var connectionString = "...";
    
    // データベースへの指示
    var sql = "SELECT user_name FROM hoge";
    // 戻り値を生成する
    var result = new List<string>();

    // 接続先からDB接続を取得してデータを結果に追加する
    using (var connection = new SqlConnection(connectionString))
    {
        connection.Open();
        using (var command = new SqlCommand(sql, connection))
        {
            using (var reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    result.Add(reader.GetString(0));
                }
            }
        }
    }
    return result;
}

一言で言えばSOLID原則の単一責任原則違反をしているということです。

以下がわかりやすいので、詳しく知りたい方は以下を参考にしてください。

今回は、単純にいろんな事をし過ぎているということが問題でしょう。
関数名をFetchUserNamesとしているが、よく考えてみてください。
正しくこの関数を言い表すのであればIDefinedDestinationInfoByHogeTableWithUserNameListAcquisition3というのが正しいわけです。

つまり関数名はその関数の役割を説明するという目的をそもそもクリアしてない事が伺えます。

よって以下のように書き換えればそれぞれの役割が真っ当できるでしょう。

public class HogeRepository
{
    private readonly string _connectionString;

    // インスタンス生成時に接続先情報を受取
    public UserRepository(string connectionString)
    {
        _connectionString = connectionString;
    }

    // ユーザー名一覧をDBから取得するための関数
    public List<string> FetchUserNames()
    {
        string sql = "SELECT user_name FROM hoge";
        return ExecuteQueryWith(
            sql,
            reader => 
            {
                var result = new List<string>();
                while(reader.Read())
                {
                    result.Add(reader.GetString(0));
                }
                return result;
            }
        );
    }

    // 接続して取得したデータをハンドラに一任する
    private T ExecuteQueryWith<T>(string sql, Func<SqlDataReader, T> handler)
    {
        var result = new List<T>();
        using (var connection = new SqlConnection(_connectionString))
        {
            connection.Open();
            using (var command = new SqlCommand(sql, connection))
            {
                using (var reader = command.ExecuteReader())
                {
                    return handler(reader);
                }
            }
        }
    }
}

上記は

  1. 接続先の指定
  2. データベースへの接続
  3. ユーザー名一覧の取得

と独立した状態になっています。

これを利用する際は以下のようになるため、IDefinedDestinationInfoByHogeTableWithUserNameListAcquisitionのような長い名前は必要なくなります。

static void Main(string[] args)
{
    // 接続先を定義し、Hogeテーブルを操作する事がわかる
    var hogeRepository = new HogeRepository("...");
    
    // hogeテーブルからユーザー名一覧を取得することがわかる
    var userNames = hogeRepository.FetchUserNames();
}

また、上記ではFetchUserNamesしか実装していないが他のデータを取得する際でも同様にFetchHogeByIdのような処理を同様に書くことができるし、
実行したSQLの実行に失敗した場合はログを出力するという要件が追加された場合でも、以下のような実装でFetchUserNames, FetchHogeById両方にログ出力の機能を付与することができます。

public class HogeRepository
{
    ...
    
    // ユーザー名一覧をDBから取得するための関数
    public List<string> FetchUserNames()
    {
        ...
    }

    // IDからHogeのオブジェクトを取得する
    public Hoge? FetchHogeById(string id)
    {
        // 本来はインジェクション対策が必要なので真似しないでね
        var sql = $"SELECT * FROM hoge WHERE id = {id}";
        return ExecuteQueryWith(
            sql,
            reader => 
            {
                if(!reader.Read())
                    return null;                
                return new Hoge {
                    id = reader.GetString(0);
                    userName = reader.GetString(1);
                    emailAddress = reader.GetString(2);
                };
            }
        );
    }

    // 接続して取得したデータをハンドラに一任する
    private T ExecuteQueryWith<T>(string sql, Func<SqlDataReader, T> handler)
    {
        var result = new List<T>();
        using (var connection = new SqlConnection(_connectionString))
        {
            connection.Open();
            using (var command = new SqlCommand(sql, connection))
            {
                try
                {
                    using (var reader = command.ExecuteReader())
                    {
                        return handler(reader);
                    }
                }
                catch(Exception ex)
                {
                    // 例外が発生した場合にログ出力をしなければならない場合は
                    // ここでログ出力を追加するだけ
                    Log.Error(ex);
                    throw;
                }
            }
        }
    }
}

最初に提示したコードであればこの命令を２つの関数に書く必要があり、片方にのみ実装し、もう片方に実装し忘れるなどのことも考えられます。

まとめ

簡単にまとめるのであれば、英語が読める人であれば関数名やクラス名を見れば何をしているかわかるコードを書こうという話です。
ある意味、英語が全くわからない人同士のコミュニティであれば、そのコミュニティで使われている言語で書くのも正直悪くないように思えます。

これは、コードを書く上でよく勘違いしている人がいると思いますが、コンピューターにわかる命令を書くのがコーディングというわけではなく、そもそもコンピューター言語自体が人間に理解できる形式へと直された言語で、人間とコンピューターが双方理解できるようにしたものがソースコードです。
そこで、動けば良いというコードを書くというのはバイナリを書いているのと変わらない状態になっているということでしょう。

また、今回クソコードとして紹介するコードは特定の接続先から特定のテーブルの特定の条件のデータを取得するということが一つの関数として実装されていました。
しかし、実際のクソコードは一つの関数内でモノリス4的に書かれていてデータベース接続からデータ取得まで全て一つのイベント内で実装しているものも見受けられます。
つまり今回はマシなクソコードということです。

そして、私達もクソコードを量産する可能性がありますし、私はすでに量産したクソコードがたくさんあります。

しかし、それはある意味仕方がないことです。

上記の例では根本的にジェネリクスの知見がないだけで、作成することは不可能ですし。知識がなければ違和感にも気づくことができないでしょう。
なので、指摘して直してもらったりすることや、問題提起をして自分でリファクタリングしていくことで保守をしやすくしていくことが重要だと思います。

当たり前ですが、人に向かって「このクソコード書いたの誰だよ」とは言うのは絶対にやめましょう。

全くこのクソ記事書いたの誰だよ。（自分）

参考にした本

  1. 多くのというのは筆者が複数の記事などから感じた所感です。(実際のサンプル数N=2) ↩

  2. 筆者が実際に見たクソ定数名。「出力状態が出力された」ことを示しているのではないか？と考察が進んでいる。なお、筆者は一生ネタにするつもりである。 ↩

  3. 「俺が定義した接続先情報でHogeテーブルからUserName一覧を取得」をCodicで翻訳したもの。DeepLで再翻訳すると「ユーザー名リスト取得のHogeテーブルで宛先情報を定義してみた」みたいになる模様 ↩

  4. モノリス(一枚岩)プログラミングという概念がある。名前付き抽象化をせず、ライブラリをほとんど使用しないものらしい。オライリーの「プログラミング文体練習」中で登場して筆者は知った。 ↩

7
1
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
7
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?