名著「リファクタリング」に沿って、リファクタリングせよッ!!! というのは難しいので、具体的なポイントを出します。
この前に前提条件があって、
1.NUnit で程よく、テストコードが書かれていること。
2.バージョン管理ツール(VSSやSVNなど)が導入されていること。
が条件になります。
「程よく」Nunit が使われているというのは、関数の中身を弄るので NUnit のテストコードを通しながらリファクタリングしたい、ということです。注意深くやれば特にテストコードはいらないのですが、手順を間違えたとき（人為的なミスですね）の発見が早くなります。
同時に、間違えたソースを元に戻すのにバージョン管理ツールが必要です。実はこれも必須というわけではなく、自前でバッグアップを取りながらでもよいのですが、修正前の差分や、一気にバージョンを戻してしまうことも可能なので、バージョン管理ツールがあると安全になります。
どちらも、【セーフティネット】の役割が強いです。
さて、リファクタリングと言えば、普通は関数の共通化が主な作業になるのですが、今回は【コード自体の質】を上げることに注力します。と言うのも、ぽろぽろと妙な書き方をされているところが多く、コードが統一されていないので、後からの保守が大変かつシステム試験時に顧客要望を取り入れにくい（コードを早急に改修しないといけない場合に、時間が掛かってしまう）という欠点があります。
・デバッグしやすいコードに直し、デバッグ時間を減らす。
・保守し易いコードに書き直し、保守可能なコードにする。
のが主目的です。
という訳で、リファクタリング対象の中で、公開しても差し支えないところをいくつか晒しておきましょう。コードは Visual Basic なのですが、C# にもあてはまります。
■if 文に not を使わない
関数が Function Method() As Boolean で作成されているときに、True/False で返すものだから、
if obj.Method() Then
...
end if
とやりたくなるのですが、
if not obj.Method() Then
...
end if
のコードが散見されたので、True/False 付きに変えることにしました。
if obj.Method() = True Then
...
end if
あるいは
if obj.Method() = False Then
...
end if
にして、明示的に boolean と比較します。こうすることで、デバッグ時にどちらで比較しているのか一瞬で分かるようになります。
まぁ、主原因は↓なコードがあって、何をやっているやら…って感じだったのが本音です。
if not obj.Method() <> 1 Then
...
end if
■if 文内で end sub/function した時は else を書かない
具体的には、
if param < 0 Then
' 異常系
...
end sub
else
' 正常系
...
end if
よくある、if 文でパラメータなどをチェックしてエラーをはじくという処理なのですが、else の前で exit sub(return なども) をして関数を抜けています。なので、else のところって意味がないですよね。
普通に↓な風に書きましょうよ、という話です。
if param < 0 Then
' 異常系
...
end sub
end if
' 正常系
...
実は、これ、えらいコードがあって、↓なコードがありました。
if param1 < 0 then
' 異常処理
end sub
else
if param2 < 0 then
' 異常処理
end sub
else
if param3 < 0 then
' 異常処理
else
' 正常処理
...
end if
end if
end if
正常系の処理をやりたいのやら、異常処理がやりたいのやら分かりません…ってな具合。
■関数の戻り値は意味のある場合だけチェックする
ちょっとローカルルール臭いのですが、こんなコードになります。
function Method() as Boolean
...
if ... then
' エラーの場合、例外を発生させる
throw new Exception(...)
end if
return true
end function
という関数があります。戻り値が True しかないのに、Function にしているのがおかしい、のは確かに言えて、本来は sub にしますね(C# ならば void型)。
これを利用するときに、律儀に、
if obj.Method() = false then
' 異常系
...
end sub
end if
しているコードが結構あるのですが、まぁ、Function だし戻り値を確認しないといけないのは筋なんですが…。これをいざ、保守しようとしたときに Method が True/False を返すのか? って調べないと駄目なんですよね。
なので、内部で例外しか発生させていないし、その例外は catch しているわけではないので、単純な処理の記述として書き換えます。
obj.Method()
実は、今回のソースコードのローカルルールで「基本的に例外を発生させない」というものを作りました。DB を扱うので、SqlCommand 関係のエラー（タイムアウト等）しか発生しないので、この手の例外はもっと外部のほうで取っています。
という理由があって、例外処理をごちゃごちゃやりたくなかった、というのがあってのリファクタリング対象です。
まぁ、本来は Fuction じゃなくて、Sub にするのが正式ですかね。
■DataTable.Rows(0) を直接参照しない
今回は、DAO という形でデータベースアクセスをするクラスを必ず作りました。
O/R マッピング的に言えば、DAO 自体にメソッドを追加するのですが、設計時間的に余裕がなかったので、そのあたりは飛ばして、DataTable を直接扱うところが多々あります（実際には、自作の型付DataTableに自動マッピングさせていますが）。
この中で、主キーなどで検索した場合、必ず1件あるとう想定で書かれているコードが結構あったので、これをリファクタリング対象にしています。
dim dt as new datatable
da.fill( dt )
dim id as integer = dt.rows(0)("ID")
dim name as string = dt.rows(0)("NAME")
...
データベース的に整合性があっていれば、1件は取得できるためここで落ちることはないのですが、初期データの絡みもあって（現状のコードの品質も加味して）、Rows.Count をチェックします。
dim dt as new datatable
da.fill( dt )
if dt.Rows.Count = 0 then
exit sub
end if
dim row as datarow = dt.rows(0)
dim id as integer = row("ID")
dim name as string = row("NAME")
...
一時変数の row を使うのは、dt.Rows(0) があちこちに出てこないようにするためです。
元のコードのほうが、dt.Rows(0) を参照していることが明確になるのは確かなのですが、保守するときに「それは、Rows(1) ではないのか？」という迷いがなくなります。
なので、VB の場合は With 構文を使っても ok です。
dim dt as new datatable
da.fill( dt )
if dt.Rows.Count = 0 then
exit sub
end if
With dt.Rows(0)
dim id as integer = .Item("ID")
dim name as string = .Item("NAME")
end with
■トランザクションの範囲は短くする
これはバグ含みなので、リファクタリングというよりも修正ですね。
dim trans as new SqlTransaction
try
dim cn as new SqlConnection("")
...
trans.Connection = cn.BeginTransaction()
if パラメータチェック Then
' 異常処理
return False
end if
...
trans.Commit()
catch ex as SqlException
trans.RollBack
return False
end try
return true
というコードが散見されているのです。実際には、SqlTransaction, SqlConnection は別のクラスで実装していますが、これの書き方では駄目ですね。パラメータチェックで異常処理をした後に Rollback ができていない…というか、パラメータチェックの前でトランザクションを開始しては駄目でしょう。
if パラメータチェック Then
' 異常処理
return False
end if
dim trans as new SqlTransaction
try
dim cn as new SqlConnection("")
...
trans.Connection = cn.BeginTransaction()
...
trans.Commit()
catch ex as SqlException
trans.RollBack
return False
end try
return true
こんな風に、データベースアクセスとは関係ないものは try の前に書かないと駄目です。例外が発生されない訳だし。
これはコードを量産するときに起きる間違いで、
– まずは、関数を始めたら try – catch で囲む
– データベースを更新するときは、begintransaction を書いてしまう
という思い込みが問題になっています。本来は、
– データベースアクセス時には例外が発生するので、try-catch する。
– データを更新する範囲だけを、トランザクションで囲む
という書き方にしないと…。
そんなリファクタリングの日々だったり。
