SE（たぶん）の雑感記

一応SEやっている筆者の思ったことを書き連ねます。会計学もやってたので、両方を生かした記事を書きたいと考えています。　でもテーマが定まってない感がすごい。

トップ > C# > 昔書いたクソコードが見つかったので晒してみる

昔書いたクソコードが見つかったので晒してみる

C# システム開発

こんばんは。

ちょっとフォルダの整理をしていたら、４、５年ぐらい前に作ったツールのソースがありました。
興味本位で見ていたら、中身がなかなかのクソだったので、晒してしまおう、と思いました。

ツール概要

データベースをちょっと便利に扱うツール。
SQL Server限定（Azureは無理）で、接続先を登録し、データコピー、データ比較、テーブル単位のインポートエクスポート等が行える。

例により、C#利用です。
.NET Frameworkのバージョンは4.5.2でした。
WPF製のアプリなのですが、諸事情により画面イメージは貼りません。ご了承ください。

なお、ツール作成後、SQLite等への対応を行おうと思いましたが、諦めています。*1

注意点

クラス名等、あまり説明しません。
だいたい名前通りの役割ですので、推察ください。

ソース晒し

Disposeよりusingが良いのでは？

役割：指定したコネクションで取得できるテーブル名を返す

/// <summary>
/// テーブル名を取得し、そのシーケンスを返します。
/// </summary>
/// <returns>テーブル名の列挙</returns>
public IEnumerable<string> Execute() {

    ConnectionCreator creator = new ConnectionCreator(_info);

    DataTable table = null;

    using (SqlConnection connection = creator.GetConnection()) {

        connection.Open();
        //Tablesを指定すると、テーブル名を取得してきてくれる
        table = connection.GetSchema("Tables");

    }

    if (table != null) {

        string name = "Table_Name";
        string schemaCol = "TABLE_SCHEMA";

        DataView view = table.DefaultView;
        view.Sort = name;

        table.Dispose();

        var newtable = view.ToTable();
        List<string> tableNames = new List<string>();

        foreach (DataRow row in newtable.Rows) {
            //自身が所有しているテーブルのみ返す
            if (row[schemaCol].ToString() == _info.SchemaName) {
                tableNames.Add(row[name].ToString());
            }
        }

        newtable.Dispose();

        return tableNames;
    } else {

        return Enumerable.Empty<string>();
    }
}

クソっぽい点

  • using、お嫌いですか？
    パッと見ただけでも、三か所ぐらい使える
    というか、table変数は破棄していないですね。
  • ソートのために、わざわざDataTable→DataView→DataTableの変換を行っている
    DataTable.AsEnumerableメソッド使えばいい

ノーガード戦法

役割：コンストラクタで受け取った情報をもとに、DBコネクションを作成して返す

public class ConnectionCreator {

    /// <summary>
    /// 作成したコネクション
    /// </summary>
    private SqlConnection Connection { get; set; }

    /// <summary>
    /// コネクションから作成したトランザクションを取得します。
    /// </summary>
    public SqlTransaction Transaction { get; set; }

    /// <summary>
    /// 当クラスに設定されている内容を元に、SqlConnectionクラスのインスタンスを作成します。
    /// </summary>
    /// <returns>SqlConnectionクラスのインスタンス。作成済みの場合は作成済のコネクション</returns>
    public SqlConnection GetConnection() {

        if (Connection == null) {
            //コンストラクタで受け取った情報で、コネクション作成
            string connectStr = CreateConnectionString();

            Connection = new SqlConnection(connectStr);
        }

        return Connection;
    }
}

クソっぽい点

  • Transactionを外から受け取れるという誰得仕様
    いや、お前が返せよ、と言いたくなること請け合い
  • ConnectionCreatorという名前以上のことをやっている
    トランザクションの管理はやめましょう

データの削除→挿入という操作があるのですが、そのときにコネクションとトランザクションを引き継ぎたかったんでしょうね。
どう考えても、外から受け取れるという仕様はアウトです。

処理が一緒だから継承しちゃおうね（しちゃだめです）

処理：書いてある通り、継承元

/// <summary>
/// データベースアクセスを行うクラスの継承元です。
/// </summary>
public abstract class DatabaseControlManagerBase {

    protected DatabaseControlManagerBase(ConnectionInfoItem conn) {

        ConnectionItem = conn;
    }

    /// <summary>
    /// コネクション情報を取得します。
    /// </summary>
    public ConnectionInfoItem ConnectionItem { get; private set; }

    /// <summary>
    /// 作成したコネクションに関する情報を取得します。
    /// </summary>
    public ConnectionCreator Connection { get; private set; }

    /// <summary>
    /// コネクション作成
    /// </summary>
    /// <returns></returns>
    protected SqlConnection CreateConnection() {
        Connection = new ConnectionCreator(ConnectionItem);

        return Connection.GetConnection();
    }
}

クソっぽい点

  • インスタンスを作れない意味が分からない
  • 抽象メソッドがないのに、何のためのabstractなのか
  • 結局CreateConnectionが欲しいだけのように見える

なお、当クラスを継承しているのは、１クラスしかありません。

お手本のような、継承の悪手です。

人が処理できる情報を超えようとしている

役割：テーブルのデータを詰め込んだDataSetを返す

/// <summary>
/// テーブルを取得します。
/// </summary>
/// <param name="act">テーブル取得通知</param>
/// <returns></returns>
public DataSet GetTables(Action<string> act) {
    using (DataSet ds = new DataSet(_info.DatabaseName)) {

        ConnectionCreator creator = new ConnectionCreator(_info);

        using (SqlConnection connection = creator.GetConnection()) {

            //コネクション確立する前に、クエリを作成しておく（開いた後はすぐ処理を終わらせたい）
            var commands = _tableNames
                .Select(t => new {
                    CommandText = DatabaseConnectionUtility.CreateSelectAllQuery(_info, t),
                    TableName = t
                })
                .OrderBy(a => a.TableName)
                .ToArray();

            connection.Open();

            try {

                foreach (var command in commands) {
                    using (SqlCommand select = new SqlCommand(command.CommandText, connection))
                    using (SqlDataAdapter adapter = new SqlDataAdapter(select)) {
                        //データベース内のテーブル名と同じ名前にしておく必要がある
                        using (DataTable table = new DataTable(command.TableName)) {
                            adapter.FillSchema(table, SchemaType.Mapped);
                            using (DataTable changeTable = CreateChangeTypeTable(table)) {
                                adapter.Fill(changeTable);
                                ds.Tables.Add(changeTable);
                                if (act != null) {
                                    act(string.Format("{0} テーブルのデータを取得しました。（行数：{1}行）", command.TableName, changeTable.Rows.Count));
                                }
                            }
                        }
                    }
                }

            } finally {

                connection.Close();
            }
        }


        DataTrimEnd(ds);

        return ds;
    }
}

クソっぽい点

  • ネストがやばい
    using使ってないという問題点がありつつ、この使いっぷり。筆者は何を考えていたのか
  • 「先にCommandを作る」という、よく分からない処理
    これによって処理が早くなったかは…神のみぞ知る

9階層ぐらいありますね。クソです。
せめて、テーブル単位の処理を切り出せていれば…という感じです。

最大の汚点

このソース、単体テストが定義されていません！
まあ、メソッドの中にSqlConnectionなんかが堂々と出てくるあたり、お察しなのですが。

Interfaceすら、一つもありません。
抽象化しようとする意図すら感じられません。*2

まとめ

まだまだ、クソさを書くと終わらないので、打ち切ります。
軽く、落ち込みます。

でも、今ならこれより良いソースを書けるようになったと思うので、良しとします。
自分にもこんな時代があったのかと、しみじみ考えてしまいました。

このソースを見て、「じゃあどうすればいいのか」という話になると思います。
それはまたの機会に書きたいと思っています。

これが現状のクソコードだとして、いきなりソースの修正に入るのは悪手ですので。
TDDに倣い、修正してみたいと考えています。

最後に一つ。
このソースは、筆者が以前に一人で作ったもので、ボロクソに書いていますが、職場やネットでこんなソースを見かけても、面と向かって罵倒しないようにしましょう。
罵倒は何も生みません。罵倒する前に、どうすればよくなるかしっかり考えてみましょう。*3

ではでは。

*1:あまり価値を見出せなかった点もある

*2:当時はそんな能力なかった

*3:特に、技術的負債となりそうな場合は、修正が必須なので、修正する方向にうまく持っていきたいところ