SIerの現場で出会ったヤバいコードをリファクタリングしてみた

皆さん
こんにちは、こんばんは
TomoProgです。

今回はSIerの現場で実際に出会ったこれはヤバいと思ったコードをリファクタリングしてみようと思います。

俺だったらこうする！
そこはこうすべきなのでは？
という意見は大歓迎ですので、何かあればぜひコメントでお知らせください。

それでは始めていきましょう。

# ヤバいコード
ヤバいと思ったコードは下記のコードです。
（実際はDBアクセスやエラー処理などがあるため、もっと複雑ですが必要な部分だけを書いています。）

```cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace SampleApp
{
    class Program
    {
        static void Main(string[] args)
        {
            // パスワードをチェックしたいときはCheckメソッドを使う
            PasswordUtil.Check("001", "password");

            // パスワードを変更したいときはUpdateメソッドを使う
            PasswordUtil.Update("001", "password", "new_password");
        }
    }

    /// <summary>
    /// パスワード関連のユーティリティクラス
    /// </summary>
    public static class PasswordUtil
    {
        public static bool Check(string userId, string password)
        {
            return CheckAndUpdate(userId, password, password, false);
        }

        public static void Update(string userId, string oldPassword, string newPassword)
        {
            CheckAndUpdate(userId, oldPassword, newPassword);
        }

        private static bool CheckAndUpdate(string userId, string oldPassword, string newPassword, bool flag = true)
        {
            // DBからユーザを取得する処理
            // （実際のコードはもっと複雑ですが、簡略化しています）
            var user = User.FindById(userId);

            bool result = false;
            if (user.Password == oldPassword)
            {
                if (!flag)
                {
                    result = true;
                }
                else
                {
                    user.Password = newPassword;
                    user.Update();
                    result = true;
                }
            }

            return result;
        }
    }
}
```
パスワード用のユーティリティクラスがあり、そのクラスのパブリックメソッドを呼ぶことで、パスワードのチェック処理や変更処理を行うようです。

# ヤバい点を整理する
ここでヤバいと思った点を整理してみます。

### 変更が容易にできない
`CheckAndUpdate`メソッドは`Check`用の処理と`Update`用の処理を担っているため、このメソッドに修正、機能追加が必要になった場合に影響範囲が大きく容易に修正できない状態になっています。
`Check`メソッド、`Update`メソッドに分けて考えることができれば、それぞれのメソッドを使用している場所のみ影響調査すればよいはずです。
さらに、分けて考えることで、`Check`、`Update`をほかのメソッドで使用したい場合に再利用が可能となります。

### メソッド名から動きが推測できない
`CheckAndUpdate`メソッドは名前からすれば、パスワードを何かしらチェックして、エラーが無ければそのままアップデートしてくれそうに思いますが、引数のフラグを制御することで挙動を変えることができてしまいます。
（この`CheckAndUpdate`メソッドを本当に命名するとすれば、`CheckOrCheckAndUpdate`でしょうか。）
メソッドの名称と内部のロジックが違うとなると、メソッド内のすべてのコードを読むこととなり、結果として読むことに工数が費やされてしまいます。

# リファクタリングする
実際にヤバい点が修正されるようにリファクタリングしていきます。

### 複数の機能を小さい機能に分割する
`CheckAndUpdate`メソッドは`Check`と`Update`という複数の機能を持ってしまっています。
これらの機能を分割して考えてみましょう。

```cs
// Check処理
user.Password == oldPassword
```

```cs
// Update処理
user.Password = newPassword;
user.Update();
```

`Check`処理に関連する部分と`Update`処理に関連する部分を抜き出しました。
抜き出すことができたらあとはこれをメソッド化するだけです。

```cs
bool Check(string userId, string password)
{
    var user = User.FindById(userId);
    return user.Password == password;
}
```

```cs
void Update(string userId, string password)
{
    var user = User.FindById(userId);
    user.Password = password;
    user.Update();
}
```

つまり、`CheckAndUpdate`メソッドは小さな機能に分割してあげれば以下のように書けるはずです。

```cs
bool CheckAndUpdate(string userId, string oldPassword, string newPassword)
{
    if (!Check(userId, oldPassword))
    {
        return false;
    }

    Update(userId, newPassword);
    return true;
}
```

大きなメソッドを小さな機能に分割することで、可読性が高いメソッドに変わりました。
さらに、嬉しいことに`Check`メソッド、`Update`メソッドが独立されており、他のメソッドから容易に再利用する事ができるようになりました。

### 分割を適用していく
上記のように`CheckAndUpdate`メソッドは分割できることがわかったので、ヤバいコードに分割を適用していきます。
ヤバいコードにはすでに`Check`と`Update`が存在しているため、そのメソッドの中身を先程、分割したメソッドの中身に置き換えてみます。

```cs
public static class PasswordUtil
{
    public static bool Check(string userId, string password)
    {
        var user = User.FindById(userId);
        return user.Password == password;
    }

    public static void Update(string userId, string oldPassword, string newPassword)
    {
        var user = User.FindById(userId);
        user.Password = newPassword;
        user.Update();
    }

    private static bool CheckAndUpdate(string userId, string oldPassword, string newPassword, bool flag = true)
    {
        if (!Check(userId, oldPassword))
        {
            return false;
        }

        Update(userId, newPassword);    // コンパイルエラー
        return true;
    }
}
```

残念ながら置き換えるだけでは、うまくいかないようです。

`Update`メソッドの引数がヤバいコードと先ほど分割したコードで異なるため、コンパイルエラーとなってしまいました。
これを解消しないといけませんが、よく見ると`CheckAndUpdate`メソッドは`private`ですし、内部のメソッドからも呼ばれていないため、メソッドごと消してしまって問題なさそうです。

```cs
public static class PasswordUtil
{
    public static bool Check(string userId, string password)
    {
        var user = User.FindById(userId);
        return user.Password == password;
    }

    public static void Update(string userId, string oldPassword, string newPassword)
    {
        var user = User.FindById(userId);
        user.Password = newPassword;
        user.Update();
    }
}
```

コンパイルエラーが解消され、`PasswordUtil`クラスが綺麗にメソッド分けされました。

### 本来の動きに戻す
コードが綺麗になって一件落着とはいかないようです。

`Update`メソッドはリファクタリングする前は`Check`の処理も行っていたはずです。
このままでは今までの動作と変化してしまうため、`Check`の処理を組み込みます。

```cs
public static class PasswordUtil
{
    public static bool Check(string userId, string password)
    {
        var user = User.FindById(userId);
        return user.Password == password;
    }

    public static void Update(string userId, string oldPassword, string newPassword)
    {
        var user = User.FindById(userId);

        // チェック処理を組み込んだ
        if (!Check(userId, oldPassword))
        {
            return;
        }

        user.Password = newPassword;
        user.Update();
    }
}
```

よく見ると、削除した`CheckAndUpdate`メソッドとそっくりになりました。

これにて、リファクタリング完了です。

```cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace SampleApp
{
    class Program
    {
        static void Main(string[] args)
        {
            // パスワードをチェックしたいときはCheckメソッドを使う
            PasswordUtil.Check("001", "password");

            // パスワードを変更したいときはUpdateメソッドを使う
            PasswordUtil.Update("001", "password", "new_password");
        }
    }

    /// <summary>
    /// パスワード関連のユーティリティクラス
    /// </summary>
    public static class PasswordUtil
    {
        public static bool Check(string userId, string password)
        {
            var user = User.FindById(userId);
            return user.Password == password;
        }

        public static void Update(string userId, string oldPassword, string newPassword)
        {
            var user = User.FindById(userId);

            if (!Check(userId, oldPassword))
            {
                return;
            }

            user.Password = newPassword;
            user.Update();
        }

    }
}
```

`CheckAndUpdate`が分割されたことにより、可読性がよくなり、修正しやすくなりました。
また、小さなメソッドに切り分けたことで再利用も容易になりました。

### 余談
ここで、メソッド名は`Update`なのに`Check`もしてしまっているではないかと思う人もいるかもしれません。
その通りですが、もし、この`Update`メソッドが`Check`メソッドを呼び出さないとなった場合、呼び出す前に必ず`Check`メソッドを呼び出さなければならないという制約が生まれることになります。
すべての使用箇所でそれを書くことよりは`Update`の内部に組み込んだほうがコストが低いのは明らかだと思います。

# まとめ
今回は実際に現場で見たコードとほぼ同じコードを自分なりにリファクタリングしてみました。
こんなコードが本当にあるのか？と思った人もいるかもしれませんが、SIerの現場にはこういったコードが山程あります。
こんなコードに一度も出会ったことがないという人はすごく恵まれた環境で仕事ができていると思います。羨ましい限りです。

冒頭にも書きましたが、
俺だったらこうする！
そこはこうすべきなのでは？
という意見は大歓迎ですので、何かあればぜひコメントでお知らせください。

それではまた。

TomoProg