はじめに

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

既存コードの解説

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

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

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継承とはこんなやつです。

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は以下と同じ意味になります。

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

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

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

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

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

Hoge.cpp

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

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

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

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

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

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

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

成果

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

Hoge.h

class TriggerInterface;

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

成果は以下の通りです。

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

やり残し

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

おわりに

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

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