読者です 読者をやめる 読者になる 読者になる

give IT a try

プログラミング、リモートワーク、田舎暮らし、音楽、etc.

レガシープログラマさんと一緒にリファクタリングをする、の巻

C# OOP

前回のエントリではレガシープログラマの判断項目について、書きました。
その日、仕事でレガシープログラマさんの一人が書いたプログラムを一緒にリファクタリングしました。
レガシープログラマさんと言っても、おいらより年下の女性エンジニアです。
今回のエントリではそのやりとりについて書いてみたいと思います。

元のプログラムはどんなプログラム?

そのプログラムは以下の判断項目に該当していました。

  • 複数のデータをまとめて扱う際は毎回配列を使う。配列の上限数はありえなさそうな数を指定する(1000とか)。
  • 基本データ型(stringやint)と配列だけでデータ構造を表現しようとする。
  • クラスのフィールド変数をグローバル変数のように利用する。


言語はC#2.0で、CSVを読み込んでメールを送信するプログラムです。
ただし、同じFromとToの組み合わせに対しては一通のメール内の複数のコンテンツを含めて送信します。
イメージ的にはこんな感じです。

csvの中身
MailFrom MailTo Content
Aさん Xさん おはよう
Aさん Xさん こんにちは
Aさん Yさん おはよう
Bさん Xさん こんばんは


このCSVの場合、3通のメールが送信されます。
1件目と2件目はFromとToが同じなので、一つのメールに「おはよう」と「こんにちは」を含めて送信します。


次にプログラムの改善前と改善後を示します。
ちなみにこの案件は新規開発ではなく、既存プログラムの機能追加です。
これもイメージです。実際はもっとややこしい感じで、コメントもほとんどありません。

改善前のプログラム
public class MailSender
{
    // メール送信情報テーブル
    // 10は送信先の最大件数(運用上、10通以上同時に送ることはないだろうという前提)
    // 3はCSVのカラムが3列だから3
    private string [,] mailInfoTable = new string[10, 3];
    
    private const string Separator = "|";
    
    public void SendMailFromCsvLines(string[] csvLines)
    {
        this.ReadCsvLines(csvLines);
        this.SendMailFromMailInfoTable();
    }
    
    private void ReadCsvLines(string[] csvLines)
    {
        // CSVの行数分ループをまわす
        foreach (string csvLine in csvLines)
        {
            // カラムを分割する
            string[] columns = csvLine.Split(',');
            
            // メール送信情報テーブルに取得した情報を格納する(最大10回ループする)
            for (int i = 0; i < this.mailInfoTable.GetLength(0); i++)
            {
                // 新しい行であればtrue
                if (string.IsNullOrEmpty(this.mailInfoTable[i, 0]))
                {
                    // MailFromとMailToを保存
                    this.mailInfoTable[i, 0] = columns[0];
                    this.mailInfoTable[i, 1] = columns[1];
                }
                
                // 既存行のMailFromとMailToが同じならtrue
                if (this.mailInfoTable[i, 0] == columns[0] &&
                    this.mailInfoTable[i, 1] == columns[1])
                {
                    // コンテンツをセパレータ文字列で区切って追加
                    this.mailInfoTable[i, 2] += Separator + columns[2];
                    break;
                }
            }
        }
    }
    
    private void SendMailFromMailInfoTable()
    {
        int i = 0;
        
        // メール送信情報テーブルを順番に読みこみ、なくなったら処理中止
        while (!string.IsNullOrEmpty(this.mailInfoTable[i, 0]))
        {
            // コンテンツはセパレータ文字列で区切られているので、分解してListに格納
            ArrayList contents = new ArrayList();
            foreach (string content in 
                this.mailInfoTable[i, 2].TrimStart(Separator).Split(Separator))
            {
                contents.Add(content);
            }
            
            // 既存のメール送信メソッドに送信処理を依頼する
            this.SendMail(this.mailInfoTable[i, 0],     // MailFrom
                            this.mailInfoTable[i, 1],     // MailTo
                            contents);                    // Contents
            i++;
        }
    }
    
    private void SendMail(string mailFrom, string mailTo, ArrayList contents)
    {
        // 既存メソッドなので実装は省略
    }
}


mailInfoTableというフィールド変数が

  • 複数のデータをまとめて扱う際は毎回配列を使う。配列の上限数はありえなさそうな数を指定する(1000とか)。
  • 基本データ型(stringやint)と配列だけでデータ構造を表現しようとする。
  • クラスのフィールド変数をグローバル変数のように利用する。

というアンチパターンをすべて兼ね備えてしまっています。
そのため、ReadCsvLinesメソッドやSendMailFromMailInfoTableメソッドの実装が分かりにくいものになってしまっています。


「this.mailInfoTable[i, 2] += Separator + columns[2];」というように複数のコンテンツを文字列で区切って格納しているところも強引というか、涙ぐましい工夫を凝らしたロジックになっています。(T T)


また、ReadCsvLinesメソッド内で同一のMailFromとMailToを探すロジックも、毎回配列をループで回しながら検索するので非効率です。


これを大体一時間ぐらいかけて下のようにリファクタリングしてみました。

改善後のプログラム
public class MailSender
{
    // メール送信情報を格納するDTOクラス
    private class MailInfo
    {
        public string MailFrom;
        public string MailTo;
        public ArrayList Contents = new ArrayList();
    }
    
    public void SendMailFromCsvLines(string[] csvLines)
    {
        // メール送信情報は戻り値で受け取る
        Dictionary<string, MailInfo> mailInfoTable = this.ReadCsvLines(csvLines);

        // メール送信情報は引数で渡す
        this.SendMailFromMailInfoTable(mailInfoTable);
    }
    
    private Dictionary<string, MailInfo> ReadCsvLines(string[] csvLines)
    {
        Dictionary<string, MailInfo> mailInfoTable = new Dictionary<string, MailInfo>();
        
        // CSVの行数分ループをまわす
        foreach (string csvLine in csvLines)
        {
            // カラムを分割する
            string[] columns = csvLine.Split(',');
            
            // CSVの値を変数に保存
            string mailFrom = columns[0];
            string mailTo = columns[1];
            string content = columns[2];
            
            // ディクショナリ用のキーを作成
            string key = mailFrom + mailTo;
            
            MailInfo mailInfo = null;
            if (mailInfoTable.ContainsKey(key))
            {
                // すでに同一のキーが存在していればディクショナリから取得
                mailInfo = mailInfoTable[key];
            }
            else
            {
                // 同一のキーが存在しなければMailInfoオブジェクトを新規に作成
                mailInfo = new MailInfo();
                mailInfo.MailFrom = mailFrom;
                mailInfo.MailTo = mailTo;
                
                // ディクショナリに追加する
                mailInfoTable.Add(key, mailInfo);
            }
            
            // コンテンツ情報を追加する
            mailInfo.Contents.Add(content);
        }
        
        return mailInfoTable;
    }
    
    private void SendMailFromMailInfoTable(Dictionary<string, MailInfo> mailInfoTable)
    {
        foreach (MailInfo mailInfo in mailInfoTable.Values)
        {
            // 既存のメール送信メソッドに送信処理を依頼する
            this.SendMail(mailInfo.MailFrom, mailInfo.MailTo, mailInfo.Contents);
        }
    }
    
    private void SendMail(string mailFrom, string mailTo, ArrayList contents)
    {
        // 既存メソッドなので実装は省略
    }
}

まあこの状態でも色々とツッコミどころはありますが、改善前よりかは可読性や堅牢性が増したはずです。
ポイントは以下の通りです。

  • MailInfoというクラスを作成し、メール送信情報のデータ構造を表現した点
  • フィールド変数を使わず、メソッドの戻り値と引数でメール送信情報を引き渡すようにした点
  • 複数のデータを格納するのに配列ではなくDictionaryクラスを使い、10件というシステム都合の上限数をなくした点
  • 同一のFromとToを検索するのに、ディクショナリのキーを利用した点

レガシープログラマさんの反応は?

リファクタリングはペアプログラミング方式というか、おいらが横について説明し、プログラムの入力はレガシープログラマさんにやってもらいました。
一通りコーディングと動作確認を行ったあとに、最後にまとめてロジックの説明しました。


自分でクラスを作ってデータを格納するようなプログラムはおそらくそれまで作ったことがないと思うので、理解してもらえるかどうか不安でしたが、大丈夫だったようです。
紙の上に図を書いたりして丁寧に説明したのが良かったのかもしれません。


そして、最後にぽつりと「感動しました」と言われました。


おいらも昔、すごい先輩の技術を目の当たりにして感動した事が何度かあります。
そうした体験が技術力を向上させたいと思う大きな原動力になりました。
おいらもそういう先輩のように誰かの手本になりたいと思いながら勉強してたりするので、非常に嬉しかったです。

2種類のレガシープログラマ

レガシープログラマの中にも昔のやり方に固執しようとするタイプの人と、単にモダンなプログラミングスタイルを知らないだけの人がいると思います。
前者の人とはぶつかりやすいかもしれませんが、後者の人はまだまだ伸びしろが大きいと思うので、こういった機会をもっと増やしていきたいですね。