デリゲート(delegate)ってなんだろう？

C#のデリゲートがよくわからんという同僚さんのために、なんかいいサンプルコードを書きたいと思ったんですが、なかなか難しいですねえ。
分かりやすくて、なおかつ実用的なサンプルが思いつかないです。

おいらの中では「デリゲート = メソッド(処理)をあたかも変数のようにして使いまわせるモノ」みたいな感じの理解です。(かなり適当・・・)

たとえば、スクリプト系の言語だと何でも実行時に解決しちゃうんで、デリゲートなんてものが必要ありません。
JavaScript(というかJScript)で書くとこんな感じです。

main();

function main() {
    var methodContainer = [];
    methodContainer.push(sayHelloInEnglish);
    methodContainer.push(sayHelloInJapanese);
    
    for (var i in methodContainer) {
        var helloMethod = methodContainer[i];
        var name = "John";
        var message = helloMethod(name);
        WScript.Echo(message);
    }
}

function sayHelloInEnglish(name) {
    return "Hello, " + name;
}

function sayHelloInJapanese(name) {
    return "こんにちは、" + name;
}

実行結果:
Hello, John
こんにちは、John

JavaScriptは関数 = オブジェクト(変数)なので、こんな書き方ができちゃいます。

// sayHelloInEnglishは関数
methodContainer.push(sayHelloInEnglish);

「メソッドを変数みたいに扱う」という感覚は、こういうスクリプト系のコードの方が分かりやすいかな〜と個人的には思います。

で、C# 2.0(古い。。。)で書くとこんな感じ。

using System;
using System.Collections.Generic;

public static class Program
{
    private delegate string SayHelloDelegate(string name);

    public static void Main(string[] args)
    {
        IList<SayHelloDelegate> methodContainer = new List<SayHelloDelegate>();
        methodContainer.Add(new SayHelloDelegate(SayHelloInEnglish));
        methodContainer.Add(new SayHelloDelegate(SayHelloInJapanese));

        foreach (SayHelloDelegate helloMethod in methodContainer)
        {
            string name = "John";
            string message = helloMethod(name);
            Console.WriteLine(message);
        }
    }

    private static string SayHelloInEnglish(string name)
    {
        return "Hello, " + name;
    }

    private static string SayHelloInJapanese(string name)
    {
        return "こんにちは、" + name;
    }
}

C#だとコンパイラが色々と事前にチェックしなきゃいけないので、何でもかんでも変数にはできません。
事前に変数化したいメソッドの引数と戻り値(いわゆるシグニチャ)をハッキリさせておかなきゃならない。
なのでJavaScriptでは出てこなかったデリゲートが必要になってきます。

このサンプルコードでは戻り値がstring、引数が1個でstringであることを「SayHelloDelegate」が規定しています。
そして「SayHelloInEnglish」と「SayHelloInJapanese」はそのシグニチャに従っています。

delegate string SayHelloDelegate(string name)

string SayHelloInEnglish(string name)

string SayHelloInJapanese(string name)

さらに、デリゲートが間に入ることでシグニチャが確定するため、コンパイラは以下のコードが実行可能であることを保証できます。

// helloMethodは変数化されたメソッド
// ここでは「SayHelloInEnglish」または「SayHelloInJapanese」のいずれか
string message = helloMethod(name);

ちなみに、無名delegateという機能を使うと「SayHelloInEnglish」や「SayHelloInJapanese」の定義が不要になります。

using System;
using System.Collections.Generic;

public static class Program
{
    private delegate string SayHelloDelegate(string name);

    public static void Main(string[] args)
    {
        IList<SayHelloDelegate> methodContainer = new List<SayHelloDelegate>();
        methodContainer.Add(delegate(string name) { return "Hello, " + name; });
        methodContainer.Add(delegate(string name) { return "こんにちは、" + name; });

        foreach (SayHelloDelegate helloMethod in methodContainer)
        {
            string name = "John";
            string message = helloMethod(name);
            Console.WriteLine(message);
        }
    }
}

少々トリッキーなコードになりますが、その処理を再利用する必要がなかったり、メソッド内のローカル変数を手軽に使い回したいときなんかには結構便利だったりします。

おいらがデリゲートを定義したり、使ったりする時のイメージはこんな感じです。
でもこんな説明でデリゲートへの理解が少しでも深まりますかねえ？
自分でもあまり自信がありませんが、何かの参考になれば幸いです。

補足: コマンドパターンで書き換えた場合

ちなみに、このロジックをあえてコマンドパターンで書き換えるとこんな感じになります。

using System;
using System.Collections.Generic;

public static class Program
{
    public static void Main(string[] args)
    {
        IList<SayHelloCommand> methodContainer = new List<SayHelloCommand>();
        methodContainer.Add(new SayHelloInEnglishCommand());
        methodContainer.Add(new SayHelloInJapaneseCommand());

        foreach (SayHelloCommand helloMethod in methodContainer)
        {
            string name = "John";
            string message = helloMethod.SayHello(name);
            Console.WriteLine(message);
        }
    }

    private abstract class SayHelloCommand
    {
        public abstract string SayHello(string name);
    }

    private class SayHelloInEnglishCommand : SayHelloCommand
    {
        public override string SayHello(string name)
        {
            return "Hello, " + name;
        }
    }

    private class SayHelloInJapaneseCommand : SayHelloCommand
    {
        public override string SayHello(string name)
        {
            return "こんにちは、" + name;
        }
    }
}

この場合、抽象クラスではなくインターフェースを使って書き換えることもできます。(だから何ということはないですが。。。)

using System;
using System.Collections.Generic;

public static class Program
{
    public static void Main(string[] args)
    {
        IList<ISayHelloCommand> methodContainer = new List<ISayHelloCommand>();
        methodContainer.Add(new SayHelloInEnglishCommand());
        methodContainer.Add(new SayHelloInJapaneseCommand());

        foreach (ISayHelloCommand helloMethod in methodContainer)
        {
            string name = "John";
            string message = helloMethod.SayHello(name);
            Console.WriteLine(message);
        }
    }

    private interface ISayHelloCommand
    {
        string SayHello(string name);
    }

    private class SayHelloInEnglishCommand : ISayHelloCommand
    {
        public string SayHello(string name)
        {
            return "Hello, " + name;
        }
    }

    private class SayHelloInJapaneseCommand : ISayHelloCommand
    {
        public string SayHello(string name)
        {
            return "こんにちは、" + name;
        }
    }
}

「デリゲートは分からないけど、コマンドパターンなら分かる！」という人にはもしかしたら参考になるかもです。

補足2: JavaScriptのコードをアレンジしてみる

前述した通り、JavaScriptでは「関数 = オブジェクト(変数)」です。
実はこんなコードを書いても動いちゃいます。

var sayHelloInEnglish = function(name) {
    return "Hello, " + name;
};

var sayHelloInJapanese = function(name) {
    return "こんにちは、" + name;
};

var main = function() {
    var methodContainer = [];
    methodContainer.push(sayHelloInEnglish);
    methodContainer.push(sayHelloInJapanese);
    
    for (var i in methodContainer) {
        var helloMethod = methodContainer[i];
        var name = "John";
        var message = helloMethod(name);
        WScript.Echo(message);
    }
};

main();

注目すべきは以下の部分です。
このコードだと関数が変数として扱われているということが明示的につかめるんじゃないでしょうか。

var sayHelloInEnglish = function(name)

var sayHelloInJapanese = function(name)

また、C#の無名delegateと同じようなコードを書くこともできます。

var main = function() {
    var methodContainer = [];
    methodContainer.push(function(name) { return "Hello, " + name; });
    methodContainer.push(function(name) { return "こんにちは、" + name; });
    
    for (var i in methodContainer) {
        var helloMethod = methodContainer[i];
        var name = "John";
        var message = helloMethod(name);
        WScript.Echo(message);
    }
};

main();

JavaScriptはJavaScriptで、興味深い仕様がたくさんあります。
詳しく知りたい方は以下の書籍がオススメです。

JavaScript 第5版

JavaScript: The Good Parts ―「良いパーツ」によるベストプラクティス

補足3: マルチキャストデリゲートを使ってみる

マルチキャストデリゲートという機能を使うと、こんな感じに書き換えられます。

using System;
using System.Collections.Generic;

public static class Program
{
    private delegate void SayHelloDelegate(string name);

    public static void Main(string[] args)
    {
        SayHelloDelegate methodContainer = null;
        methodContainer += new SayHelloDelegate(SayHelloInEnglish);
        methodContainer += new SayHelloDelegate(SayHelloInJapanese);
        string name = "John";
        methodContainer(name);
    }

    private static void SayHelloInEnglish(string name)
    {
        Console.WriteLine("Hello, " + name);
    }

    private static void SayHelloInJapanese(string name)
    {
        Console.WriteLine("こんにちは、" + name);
    }
}

さらに無名delegateを使うとこんな風になります。

using System;
using System.Collections.Generic;

public static class Program
{
    private delegate void SayHelloDelegate(string name);

    public static void Main(string[] args)
    {
        SayHelloDelegate methodContainer = null;
        methodContainer += delegate(string name) { Console.WriteLine("Hello, " + name); };
        methodContainer += delegate(string name) { Console.WriteLine("こんにちは、" + name); };
        methodContainer("John");
    }
}

さらにC# 3.0では・・・と続くはずなのですが、手軽に確認できる環境がないため、ここで切り上げます。 m(_ _)m
=> 補足5にて書き換えてみました。

補足4: なんとなくRubyで書いてみる

Rubyは全くの初心者ですが、見よう見まねで書いてみました。
単なるRubyへの憧れです、はい。

def say_hello_in_english(name)
  "Hello, " + name
end

def say_hello_in_japanese(name)
  "こんにちは、" + name
end

method_container = []
method_container.push method :say_hello_in_english
method_container.push method :say_hello_in_japanese

method_container.each do |hello_method| 
  name = "John"
  message = hello_method.call name
  puts message
end

無名delegateっぽく書くとこんな感じ？

method_container = []
method_container.push lambda{|name| "hello, " + name}
method_container.push lambda{|name| "こんにちは、" + name}

method_container.each do |hello_method| 
  name = "John"
  message = hello_method.call name
  puts message
end

補足5: C#3.0で書き換えてみる

今度はC# 3.0で書き換えてみました。
ラムダ式ってのを使うとdelegateの宣言がいらなくなくなるみたいですね〜。
ループ処理もRubyみたいな書き方ができるので、ソースがさらに簡潔になります。
会社の環境もさっさと最新の開発環境に移行してもらいたいのですが。う〜ん。。。

using System;
using System.Collections.Generic;

public static class Program
{
    public static void Main(string[] args)
    {
        var methodContainer = new List<Func<string, string>>();
        methodContainer.Add((name) => "Hello, " + name);
        methodContainer.Add((name) => "こんにちは、" + name);

        methodContainer.ForEach((helloMethod) =>
            {
                var name = "John";
                var message = helloMethod(name);
                Console.WriteLine(message);
            });
    }
}

元のロジックを意識しつつ、とことん短く書いてやろうと思うとこんな感じ？
かなりトリッキーで一体何をやりたいのかわからないかもしれませんが。。。 (^ ^;;

using System;
using System.Collections.Generic;

public static class Program
{
   public static void Main(string[] args)
   {
       new List<Func<string, string>>
       {
           ((name) => "Hello, " + name),
           ((name) => "こんにちは、" + name)
       }
       .ForEach((helloMethod) =>
       {
           var name = "John";
           var message = helloMethod(name);
           Console.WriteLine(message);
       });
   }
}

補足6: F#で書き換えてみる

調子に乗ってF#版まで作ってみました。
全くもって関数型脳は習得できていないので、たぶん全然関数型言語らしくないんじゃないかと思います・・・。
あまり参考になさらぬよう。(- -;;

let printMessage helloFunc =
  let name = "John"
  let message = helloFunc name
  printfn "%s" message

List.iter printMessage [(fun name -> "Hello, " + name); (fun name -> "こんにちは、" + name)]

可読性を無視したらこうなりました。ただの変態コードですね。。。

List.iter 
  (fun helloFunc -> printfn "%s" (helloFunc "John"))
  [(fun name -> "Hello, " + name); (fun name -> "こんにちは、" + name)]

C#のコード品質を上げるフリーツール8本

はじめに

読みにくいコードや複雑なコードをメンテナンスするのってイヤですよね。
コードの品質を上げる方法の一つにコードレビューがありますが、すべてのソースコードを人力でチェックしていくのは大変ですし、レビュアーのスキルや好みにも大きく依存してしまいます。

そういう場合はツールを使って自動化するのが有効です。
ツールを使えばあっという間に完了しますし、実施者のスキルや好みに左右されることもありません。
しかし、あまりお金がかかるツールだと、ちょっと気軽に導入しにくいです。

そこで今回はC#のコード品質向上に有効なフリーツールを紹介します。
実際のプロジェクトで使用したことがあるものばかりなので、どれも「使えるツール」だと思いますよ。

ところで、ツールを紹介する前にTipsと注意点を簡単に挙げておきましょう。

ツールを利用する際のTips

• 自分の書いたコードのみを対象とし、ツールが作成したコードは対象外にしましょう。Projectやネームスペースを別にして、自分の書いたコードとツールが自動生成したコードを簡単に識別できるようにしておくことが重要です。
• ただし、ヘルプファイル(APIドキュメント)作成時は自動生成されたコードも出力対象にしてよいと思います。(そこだけ不完全なヘルプになるかもしれませんが)
• UIはテストしにくいので、自動化テストやコードカバレッジの対象外とすることを検討すると良いかもしれません。もちろん、主要なロジックはUIの外に切り出す設計にしておくことが重要です。
• 自分の組織、または開発プロジェクトでツールのバージョンやオプションを統一しておきましょう。実行したメンバーやPCによって結果が異なると、ツールの存在意義が低下してしまいます。たいていのツールには設定ファイルがあるので、それをバージョン管理ツールなどで共有すると良いと思います。
• ツールの有効性が確認できたら、さらに高機能な有料ツールの導入を検討するのもいいでしょう。

これから紹介するツールの注意点

• 確認した環境はVisualStudio 2005/C# 2.0です。2008/3.5や2010/4.0での動作確認はできていません。*1
• ツールのトップページは時々変わるので、時間が経つとリンクが切れている場合があります。リンクが切れていなくても、最新版のトップページが全く違うURLになっていたりすることもあるので、Googleなどで最新情報を検索しなおすことをお勧めします。
• ツールの中には開発があまり活発ではないものもあります。開発環境がアップデートされてそのツールが使えなくなるリスクもある程度考慮しておく必要があります。
• 今まで無料だったツールが突然有料になったりすることがあります。まあ、ボランティアでツールを開発している方はいないと思うので、そのときは諦めてください。
• 使い方についてはここでは詳しく書きません。公式サイトの情報やネット上の記事などを参考にしてください。特にWebSiteは他のProjectと大きく構成が違うのでツールが適用しにくいことがあります。

それではツールの紹介に移りましょう。

C#のコード品質を上げるフリーツール

StyleCop

Project内のソースコードがコーディングルールに従っているかチェックしてくれます。
時々、VisualStudioがデフォルトで作成するコードからすでにルール違反が含まれていたり、日本語環境ではどうしようもないルール違反を報告されたりするので、自分のProjectにあわせてルールのオプションを変更してください。

公式サイト

http://archive.msdn.microsoft.com/sourceanalysis

FxCop

ソースコードではなく、アセンブリ(dll/exe)の品質チェックをしてくれます。
クラス名やメソッド名にスペルミスがあったりすると教えてくれます。
ただし、そのアセンブリを他のProjectで使うのでなければチェックが厳しすぎるところもあるので、自分のProjectにあわせてルールのオプションを変更してください。

公式サイト

http://www.microsoft.com/downloads/en/details.aspx?displaylang=en&FamilyID=917023f6-d5b7-41bb-bbc0-411a7d66cf3c

SourceMonitor

サイクロマチック複雑度やネストの深さなどをチェックしてくれます。
コードのシンプルさ、複雑さを定量化することができます。

公式サイト

SourceMonitor V3.5

Duplo

コードの重複を発見してくれます。
ただし、実行するには自分でC++のコードをコンパイルする必要があります。
異なるファイル間の重複は発見してくれますが、同一ファイル内の重複は発見してくれないのがちょっと難点です。
ファイルの一覧をテキストファイルとして作成し、それをDuploに食わせるので一発では解析できません。
おいらは一発で解析できるように、自動化バッチ(厳密にはhtaで作ったGUI)を作成しました。

公式サイト

http://sourceforge.net/projects/duplo/

NUnit

言わずと知れた自動化テストツールです。
VisualStudioから起動＆デバッグ実行ができるように、ProjectのBuildプロパティを変更しておきましょう。

公式サイト

NUnit.org

PartCover

NUnitと組み合わせてコードカバレッジを解析します。*2
実行時のプロパティ設定にちょっとクセがあるので、最初は少し苦労するかもしれません。

公式サイト

GitHub - sawilde/partcover.net4: Convert the PartCover CodeCoverage tool to .NET4 - follow us on Twitter @partcover

GhostDoc

コンテキストメニューやキーボードショートカットを使って簡単にXMLドキュメント・コメントを作成できます。
クラスやメソッドに適切な英語名が付いていると、それっぽい英文コメントも自動生成されます。
まあ、日本語でコメントを書きたい場合は無用な機能になってしまいますが・・・。

公式サイト

SubMain / GhostDoc - Painless Help Documentation

Sandcastle

XMLドキュメント・コメントからヘルプファイル(APIドキュメント)を作成してくれます。
オプションが豊富にあります。逆に言うと、オプションが多すぎて何をどう設定すればいいか迷うかもしれません・・・。
昔はNDocという似たようなツールがありましたが、開発が止まっているようなので、Sandcastleを使った方が良いと思います。

公式サイト

CodePlex Archive

ツールを使えばすべて完璧？

いえいえ、そんなわけはありません。
これらのツールを使えば、可読性や信頼性といったコードの品質が向上するのは間違いありません。
しかしそうはいっても、これらのツールがコードの品質を完璧にしてくれるわけでもありません。
ツールを使ってもツールではチェックできない部分も色々とあるので、人力のコードレビューもある程度必要になると思います。
もちろん、それが機械的に判断できる内容なのであれば、他のツールを探したり、自分たちでチェックツールを作ったりすればさらに完璧ですね。

手段と目的を取り違える人もいる

たとえば実際の開発プロジェクトでメトリクスの目標値を決めたとします。
コードカバレッジは100%とか、StyleCopの検証をすべてパスさせるとか。
しかし、開発者がその気になればツールを欺くこともできます。
カバレッジは100%でもNUnit側で大した内容を検証していなったり、適当なXMLドキュメント・コメントしか入れてなかったりという具合です。
残念ですが、開発に参加する人の中にはそういう悪知恵を働かす人間が紛れ込むこともあるので、ツールが値だけを鵜呑みにせず、時々自分の目で確かめることもしましょう。

あわせて読みたい

SourceMonitorの活用事例を紹介しています。
数値で測るコード品質 - give IT a try

テストを自動化する際の注意点をまとめてみました。
自動化テストで気をつけること - give IT a try

ツールを使ってもレガシープログラマの判断項目をスルーしてしまう恐れはあるので要注意！
レガシープログラマかどうかを判断する10項目 - give IT a try

ツールの測定値を組織の指標とするときは、こうした弊害にも注意しましょう。
おかしなおかしな目標管理 - give IT a try

参考文献

ここで挙げたツールはこの本にインスパイアされて探し出したものです。
最終的にはNAntやCruiseControlなんかを使ってCI(継続的インテグレーション)までできれば完璧なんですが。

継続的インテグレーション入門

.NETライブラリの開発者が自らライブラリの設計指針を説明してくれます。
.NETの命名規則や設計ポリシーを学ぶのに打ってつけです。
ただし、訳がかなり読みにくいのが難点(> <)

.NETのクラスライブラリ設計　開発チーム直伝の設計原則,コーディング標準,パターン

C#の標準的なコーディングルールがうまくまとめられています。
コーディングルール検証はツールに任せっきりにするのではなく、こうした本を読んで、どういったルールがあるのか、なぜそのルールを適用するのか、といったことも理解しておきましょう。

超図解 C#ルールブック (超図解シリーズ)

*1:古い環境でごめんなさい m(_ _)m

*2:厳密に言うとNUnitである必要はありません

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

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

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

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

• 複数のデータをまとめて扱う際は毎回配列を使う。配列の上限数はありえなさそうな数を指定する(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種類のレガシープログラマ

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