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

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

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

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

複数のデータをまとめて扱う際は毎回配列を使う。配列の上限数はありえなさそうな数を指定する(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)
    {
        // 既存メソッドなので実装は省略
    }
}