C++ 巨大化したクラスをリファクタリングした経験

# はじめに

この記事は [Qiita Engineer Festa 2022](https://qiita.com/official-campaigns/engineer-festa/2022) のテーマ「設計」参加記事です。
最近設計に興味を持って勉強しており、業務で扱っているコードの設計をどうにかしないといけないなあと思って少しずつ改善をしているのですが、イベントで「悪いコードを改善した時の体験や知見を共有しよう」というちょうどよいテーマがあったので1つの経験を投稿することにしました。

# 既存コードの解説

紹介するコードは業務のコードなので、具体的な機能は伏せます。言語はC++です。
今回の改善の対象クラスを`Hoge`とします。

既存コードは以下のようなクラス構成でした。

```plantuml
@startuml
HogeChildA --|> Hoge
HogeChildB --|> Hoge
HogeChildC --|> Hoge
AppA --> HogeChildA
AppB --> HogeChildB
AppA --> HogeChildC
AppB --> HogeChildC

class Hoge{
+PublicFunc1()
+PublicFunc2()
+PublicFunc3()
#ProtectedFunc1()
#ProtectedFunc2()
#ProtectedFunc3()
#VirtualFunc()
-PrivateFunc1()
-PrivateFunc2()
-PrivateFunc3()
-variable
}

class HogeChildA{
#VirtualFunc()
}
class HogeChildB{
#VirtualFunc()
}
class HogeChildC{
+PublicFuncOfC()
}
@enduml
```

`Hoge`クラスを継承して3つのクラスが作られています。
3つのクラスは、アプリケーション2つ(`AppA`, `AppB`)から使用されるクラスです。
`HogeChildA`は`AppA`が使用し、`HogeChildB`は`AppB`が使用し、`HogeChildC`は`AppA`, `AppB`の両方が使用しています。
`Hoge`クラスのメンバ関数の数は図では控えめに書きましたが、**実際は30個以上あります。** クラス名は`Hoge`でなくて`Huge`の方がよかったか？

# 改善1 継承をなくす

まずクラスを見て気になったのが継承でした。継承は便利なのでつい使ってしまいますが、継承はクラス同士の結合を密にするため継承よりも委譲を使おうというのがいろんな本で言われています。

## 継承ではなくコールバックを使うように変更

`HogeChildA`, `HogeChildB`を見ると、基底クラスの仮想関数`VirtualFunc()`をオーバーライドしています。クラスによって処理を変えたいためにこのようにしてあったと考えられます。**継承している理由はこれだけのようでした**。しかも、オーバーライドした関数の中で呼び出している関数は**グローバル関数**で、ほかのクラスに依存しているわけではありませんでした。
ということで、ここをコールバックで置き換えることにしました。呼び出し元の`AppA`, `AppB`がアクセスできる関数だったので余裕でした。
これで`HogeChildA`, `HogeChildB`を**撲滅**できました。

## private継承をやめる

`HogeClassC`は`HogeChildA`, `HogeChildB`と少し違う継承のしかたをしていました。
よく見ると、これは**private継承**でした。…えっ？
private継承とはこんなやつです。

```c++
class Hoge // 基底クラス
{
public:
  void PublicFunc1();
protected:
  void ProtectedFunc1();
private:
  int variable;
  void PrivateFunc1();
};

class HogeChildC : private Hoge // private継承した派生クラス
{
public:
  void PublicFuncOfC();
};
```

正直自分はpublic継承しか使ったことがなく、private継承のコードを見るのは初めてでした。private継承すると、基底クラスのpublicメンバ関数さえも使えなくなります。

上記`HogeChildC`は以下と同じ意味になります。

```c++
class HogeChildC
{
public:
  void PublicFuncOfC();
private:
  void PublicFunc1();    // 基底クラスではpublicだったが派生クラスではprivateになる
  void ProtectedFunc1(); // 基底クラスではprotectedだったが派生クラスではprivateになる
};

```

設計者の意図は何だろうとコードをよく読んだところ、**`PublicFuncOfC()`から基底クラスのprotectedメンバ関数(`ProtectedFunc1()`)を呼び出したいだけ**ということがわかりました。
もうHogeクラスと切っても切れない関係になっているため、`PublicFuncOfC()`をHogeクラスのメンバにすることにしました。
これで`HogeChildC`クラスも撲滅できました。

# 改善2 メンバ関数を減らす

継承を撲滅できたので、クラス図は以下のようになりました。

```plantuml
@startuml
AppA --> Hoge
AppB --> Hoge

class Hoge{
+PublicFunc1()
+PublicFunc2()
+PublicFunc3()
+PublicFuncOfC()
#ProtectedFunc1()
#ProtectedFunc2()
#ProtectedFunc3()
-PrivateFunc1()
-PrivateFunc2()
-PrivateFunc3()
-variable
}
@enduml
```

気になるのは関数が多いことです。上にも書きましたが実際には**30個以上メンバ関数があります。**
その割に**メンバ変数は1個だけ**です。
そのメンバ変数を使っているメンバ関数を調べたら、なんと**2個だけ**でした。
メンバ関数を使っていない関数はこのクラスの関数である必要がないので、どんどん外していきます。具体的には、privateなメンバ関数をHoge.cppの中の無名名前空間に入れていきます。また、このクラスは継承されなくなったのでprotectedなメンバ関数も同様にできます。

```c++:Hoge.cpp
namespace {  // 無名名前空間
  void PriveteFunc1()  // privateメンバ関数から移動
  {
    // ..
  }
  void ProtectedFunc1()  // protectedメンバ関数から移動
  {
    // ..
  }
}

void Hoge::PrivateFunc3()
{
  // 内部でメンバ変数を使っているのでクラスの関数でないといけない
}

```

# 改善3 条件判定を別クラスに分離

メンバ関数を減らすことができたので以下のようになりました。

```plantuml
@startuml
AppA --> Hoge
AppB --> Hoge

class Hoge{
+PublicFunc1(trigger:enum)
+PublicFunc2()
+PublicFunc3()
+PublicFuncOfC()
-PrivateFunc3()
-variable
}
@enduml
```

だいぶスッキリしてきましたが、まだpublic関数が多いです。
ちなみにこれらのpublic関数は**だいたい似ているけど少し違う**ことをしています。**関数をコピペして編集したやろ**ということが透けて見えます。上で派生クラスから移動してきた`PublicFuncOfC`も同様です。
また、クラス図に表せませんが`PublicFunc1()`は`enum`の`trigger`を引数にとり、**その引数とグローバルな条件（アプリ設定とか）を組み合わせて複雑な条件分岐を行っています。**
ここは地道にそれぞれの関数の機能を調査して、共通の部分と違う部分を表にまとめ上げました。表にまとめたあと、**もう一段階抽象的に考えると**どういう条件で分岐しているのかを推測しました。推測した条件分岐はこのクラスの中で行うのではなく、**条件判定用のクラス（インターフェース）** を別に作ることにしました。

条件判定用のクラス（インターフェース）は以下です。

```plantuml
@startuml
interface TriggerInterface{
+ IsConditionA()
+ IsConditionB()
+ IsConditionC()
}
class Trigger1{
+ IsConditionA()
+ IsConditionB()
+ IsConditionC()
}
class Trigger2{
+ IsConditionA()
+ IsConditionB()
+ IsConditionC()
}
class Trigger3{
+ IsConditionA()
+ IsConditionB()
+ IsConditionC()
}

TriggerInterface <|-- Trigger1
TriggerInterface <|-- Trigger2
TriggerInterface <|-- Trigger3
@enduml
```

抽象化した条件を`IsConditionA()`,`IsConditionB()`,`IsConditionC()`で判断しています。こ の中でグローバルな条件の判定も行っています。
これにより、関数内部を必要以上に複雑にすることなくpublic関数をまとめることができました。
正直なところ、ここのリファクタリングが成功しているか自信がなかったのですが、**気合の手動テスト**で担保しました。

# 成果

上記の改善を行った結果、以下のようになりました。

```plantuml
@startuml
interface TriggerInterface
class Hoge{
+PublicFunc1(trigger:TriggerInterface)
-PrivateFunc3()
-variable
}

AppA --> Hoge
AppB --> Hoge
TriggerInterface <|-- Trigger1
TriggerInterface <|-- Trigger2
TriggerInterface <|-- Trigger3

Trigger1 <-- AppA
Trigger2 <-- AppA
Trigger3 <-- AppA
Trigger1 <-- AppB
Trigger2 <-- AppB
Trigger3 <-- AppB

@enduml
```

```c++:Hoge.h
class TriggerInterface;

class Hoge final
{
public:
  PublicFunc1( const TriggerInterface& trigger );
private:
  PrivateFunc3();
  variable;
};
```

成果は以下の通りです。
* `Hoge`クラスの関数を大幅に減らせました。
* 動作の条件判定を別クラス`TriggerInterface`に抽出できました。条件追加・変更の際はこちらだけ修正すれば良くなりました。
* もう継承はうんざりなので`Hoge`に`final`を付けました。

# やり残し

* Hoge.cppに残った無名名前空間内の関数に注目すると、いくつかのクラスに分類できそうな感じがしたのですが、動いているコードを意味もなく変えるのが嫌われがちな文化のため大胆な変更がまだできていません。
* 書いていませんが、`Hoge`クラスに削除できなかったpublic関数があります。どうも特定の顧客向けのコードらしく、テストできないため諦めました。
* リファクタリングを行った結果関数が減ったのですが、このクラスの役割は一体何だったんだろうという気持ちになりました。

# おわりに

本などでいろいろ勉強していますが、どんな本よりも現場のコードこそ最も生々しく、勉強材料にはうってつけだと思っています。この体験が皆さんの勇気付けになれば幸いです。私も巨大クラスに打ちひしがれながら頑張っていこうと思います。