C++
リファクタリング手法

前書き

リファクタリング

ここでは、リファクタリングの恩恵を受ける可能性のあるプログラムを紹介します。これは、C ++ 11を使用する単純なプログラムで、1から100までのすべての素数を計算して出力することを目的としており 、レビューのためにCodeReviewに投稿されたプログラムに基づいています。

#include <iostream>
#include <vector>
#include <cmath>

int main()
{
    int l = 100;
    bool isprime;
    std::vector<int> primes;
    primes.push_back(2);
    for (int no = 3; no < l; no += 2) {
        isprime = true;
        for (int primecount=0; primes[primecount] <= std::sqrt(no); ++primecount) {
            if (no % primes[primecount] == 0) {
                isprime = false;
                break;
            } else if (primes[primecount] * primes[primecount] > no) {
                std::cout << no << "\n";
                break;
            }
        }
        if (isprime) {
            std::cout << no << " ";
            primes.push_back(no);
        }
    }
    std::cout << "\n";
}

このプログラムの出力は次のようになります。

3 5 7 11 13 17 19 23 29 31 37 41 43 47 53 53 61 67 71 73 79 83 89 97

最初に気付くのは、プログラムが素数である2を印刷できないということです。プログラムの残りの部分を修正することなくその定数を単純に出力するコード行を追加するだけで、プログラムをリファクタリングして2つの部分に分割するほうが簡単かもしれません - 素数リストを作成するものと、 。それがどのように見えるかは次のとおりです。

#include <iostream>
#include <vector>
#include <cmath>

std::vector<int> prime_list(int limit)
{
    bool isprime;
    std::vector<int> primes;
    primes.push_back(2);
    for (int no = 3; no < limit; no += 2) {
        isprime = true;
        for (int primecount=0; primes[primecount] <= std::sqrt(no); ++primecount) {
            if (no % primes[primecount] == 0) {
                isprime = false;
                break;
            } else if (primes[primecount] * primes[primecount] > no) {
                break;
            }
        }
        if (isprime) {
            primes.push_back(no);
        }
    }
    return primes;
}

int main() 
{
    std::vector<int> primes = prime_list(100);
    for (std::size_t i = 0; i < primes.size(); ++i) {
        std::cout << primes[i] << ' ';
    }
    std::cout << '\n';
}

このバージョンを試してみると、実際に正しく動作していることがわかります。

2 3 5 7 11 13 17 19 23 29 31 37 41 43 47 53 59 61 67 71 73 79 83 89 97

次のステップは、2番目のif節が本当に必要でないことに気づくことです。ループ内のロジックは、与えられた数の素因数をその数の平方根まで探します。これは、数の素因数が存在する場合、そのうちの少なくとも1つがその数の平方根以下でなければならないためです。その機能だけを再構築すると（残りのプログラムは同じままです）、この結果が得られます：

std::vector<int> prime_list(int limit)
{
    bool isprime;
    std::vector<int> primes;
    primes.push_back(2);
    for (int no = 3; no < limit; no += 2) {
        isprime = true;
        for (int primecount=0; primes[primecount] <= std::sqrt(no); ++primecount) {
            if (no % primes[primecount] == 0) {
                isprime = false;
                break;
            }
        }
        if (isprime) {
            primes.push_back(no);
        }
    }
    return primes;
}

変数名をもっとわかりやすいように変更することができます。たとえば、 primecountは実際には素数のカウントではありません。代わりに、それは既知の素数のベクトルへのインデックス変数です。また、 noは "number"の略語として使われることがありますが、数学的な書き方ではnを使うのが一般的です。また、 breakなくし、変数を使用する場所に近いところで変数を宣言することで、いくつかの変更を加えることもできます。

std::vector<int> prime_list(int limit)
{
    std::vector<int> primes{2};
    for (int n = 3; n < limit; n += 2) {
        bool isprime = true;
        for (int i=0; isprime && primes[i] <= std::sqrt(n); ++i) {
            isprime &= (n % primes[i] != 0);
        }
        if (isprime) {
            primes.push_back(n);
        }
    }
    return primes;
}

私たちはまた、「レンジ・フォー」を使用してmainリファクタリングを行うこともできます。

int main() 
{
    std::vector<int> primes = prime_list(100);
    for (auto p : primes) {
        std::cout << p << ' ';
    }
    std::cout << '\n';
}

これは、リファクタリングが行われるかもしれない1つの方法です。他はさまざまな選択をするかもしれない。しかし、リファクタリングの目的は変わりません。これは、機能を追加することなく、コードの可読性とパフォーマンスを向上させることです。

ゴトクリーンアップ

CであったC ++コードベースでは、 goto cleanupのパターンを見つけることができます。 gotoコマンドは、関数のワークフローを理解しにくくするので、これはしばしば回避されます。多くの場合、リターン文、ループ、関数に置き換えることができます。しかし、 goto cleanupでは、クリーンアップロジックを取り除く必要があります。

short calculate(VectorStr **data) {
    short result = FALSE;
    VectorStr *vec = NULL;
    if (!data)
       goto cleanup;  //< Could become return false

    // ... Calculation which 'new's VectorStr

    result = TRUE;
cleanup:
    delete [] vec;
    return result;
}

C ++では、この問題を解決するためにRAIIを使うことができます：

struct VectorRAII final {
    VectorStr *data{nullptr};
    VectorRAII() = default;
    ~VectorRAII() {
        delete [] data;
    }
    VectorRAII(const VectorRAII &) = delete;
};

short calculate(VectorStr **data) {
    VectorRAII vec{};
    if (!data)
       return FALSE;  //< Could become return false

    // ... Calculation which 'new's VectorStr and stores it in vec.data

    return TRUE;
}

この時点から、実際のコードをリファクタリングし続けることができます。たとえば、 VectorRAIIをstd::unique_ptrまたはstd::vector置き換えます。