さんちゃのblog

プログラミングのことなど

機能追加と同時にリファクタリングをしてもいいか

「機能追加と同時にリファクタリングをしてもいい」という記事がはてブに上がっていたので、思うところを述べる。

scrapbox.io

結論

問題意識

機能追加をするときにリファクタは避けられない。既存の処理を共通化して再利用して機能を追加した方が効率的だからである。

しかし、機能追加とリファクタが混ざったPRを出すのはダメだ。なぜなら、リファクタによって機能が失われていないかどうか確認するのが面倒になるからだ。

解決策

では、どうすればよいのか?

開発中はリファクタと機能追加を平行して行い、リファクタと機能追加のPull Requestを別々に出せば良いのである。

手順

この解決策を実行するには以下のようにする:

  1. 開発中はリファクタと機能追加を同時並行で行い、適宜コミットする。
  2. PRを作成する前にgit rebase -iコミットを入れ替え 、「リファクタのみのブランチ」と「機能追加のみのブランチ」を作成する。
  3. それぞれのブランチについてPull Requestを作成する。
  4. レビュアーは、「リファクタのみのブランチ」をレビュー&マージした後に、「機能追加のみのブランチ」のレビューを行う。

つまり、開発完了時の

* implement B <-- some_new_feature
*  refactor Y
* implement A
*  refactor X
*        base

という歴史を、

* implement B <-- some_new_feature
* implement A
*  refactor Y <-- refactor_for_new_feature
*  refactor X
*        base

のように書き換え、2つのPull Requestを作成するのである。

このようにすれば、「機能追加とリファクタは同時にやりたい」という欲求と、「機能追加とリファクタは別にレビューしたい」という欲求を両立させることができる。

元記事に対する感想

「リファクタと機能実装を分けると、リファクタの方のPRでは「設計が壊れ」ているコードをレビューしなければいけないので良くない」みたいな記述があったが、ちゃんと意味のあるリファクタであれば、それはそれとして独立した一つのPRになると思う。「リファクタのPRは『設計が壊れ』ている」ことは、変なリファクタをしてるからなのではないかと思う。

また、「githubのクソUIのせいで、リファクタと機能実装が混ぜ込まれたPRがレビューしづらい」という意味の記述があったが、ほならねとしか言えない。そもそもdiffを出してるのは、変更範囲だけを見たいという欲求があるからであって、「全体を見てもらえばもっとわかりやすいんです!!!」というのはレビュワーに対する負荷を増やすだけだ。

iter()とinto_iter()の違いを整理した

VectorIteratorに変換する時にいつも混乱していたので整理した。

混乱

あるVectorの要素すべてを3倍するコードを考える。

fn main() {
    let vec1 = vec![1,2,3,4,5];
    let vec2 = vec1.iter()
                   .map(|i| i * 3)
                   .collect::<Vec<i32>>();

    println!("{:?}", vec1);
    println!("{:?}", vec2);
}

このコードはコンパイルできるが、以下のような疑問がある。

  1. 5行目で vec1はなぜ使えるのか? 3行目の vec1.iter()で使われているじゃないか!
  2. map(|i| i * 3)iは参照なのか値なのか?

これらの疑問に関する答えは、

  1. iter()Vectorをmoveしない。into_iter()Vectorをmoveする。
  2. iter()Vectorから「参照のコレクションであるIterator」を作成し、into_iter()Vectorから「値のコレクションであるIterator」を作成する。

ということになる。

into_iter()の挙動

呼び出し元のVectorの所有権

fn main() {
    let vec1 = vec![1,2,3,4,5];
    let vec2 = vec1.into_iter()
                   .map(|i| i * 3)
                   .collect::<Vec<i32>>();

    println!("{:?}", vec1); // Error!
}

このコードはコンパイルできない。というのも、3行目の into_iter()vec1をmoveするからである。

このように、 into_iter()は、呼び出し元のVectorをmoveする。

mapに渡される要素は参照なのか?

fn main() {
    let vec1 = vec![1,2,3,4,5];
    let vec2 = vec1.into_iter()
                   .map(|i| *i * 3) // Error!
                   .collect::<Vec<i32>>();
}

このコードはコンパイルできない。というのも、4行目の map(|i| *i * 3)iは、参照ではなく値だからである。

このように、into_iter()に連なる mapには、参照ではなくが渡される。

iter()の挙動

呼び出し元のVectorの所有権

fn main() {
    let vec1 = vec![1,2,3,4,5];
    let vec2 = vec1.iter()
                   .map(|i| i * 3)
                   .collect::<Vec<i32>>();

    println!("{:?}", vec1); // OK!
}

このコードはコンパイルできる。iter()vec1をmoveしないからである。

このように、 iter()は、呼び出し元のVectorをmoveしない。

mapに渡される要素は参照なのか?

fn main() {
    let vec1 = vec![1,2,3,4,5];
    let vec2 = vec1.iter()
                   .map(|i| *i * 3) // OK!
                   .collect::<Vec<i32>>();
}

このコードはコンパイルできる。4行目の map(|i| *i * 3)iは、値ではなく参照だからである。

このように、iter()に連なる mapには、参照が渡される。

結論

VectorIteratorに変換して処理を行いたいとき、

  • そのVectorを後で利用するならiter()を使う。その場合、mapには要素の参照が渡される。
  • そのVectorを後で利用しないならinto_iter()を使う。その場合、mapには要素の値が渡される。

速度について

一応簡単にベンチマークして、iter()into_iter()の間に速度的な差があるのか検証した。 なんとなくiter()の方が遅いような気がするが、誤差幅を考えるとほとんど差がないように見える。

ベンチマークのコード:

#![feature(test)]

extern crate test;

pub fn iter(vec: Vec<i64>) {
    vec.iter().map(|i| i * 2).sum::<i64>();
}

pub fn into_iter(vec: Vec<i64>) {
    vec.into_iter().map(|i| i * 2).sum::<i64>();
}

#[cfg(test)]
mod tests {
    use super::*;
    use test::Bencher;

    #[bench]
    fn bench_iter(b: &mut Bencher) {
        let range = 0..100000;
        let vec = range.collect::<Vec<i64>>();

        b.iter(|| iter(vec.clone()));
    }

    #[bench]
    fn bench_into_iter(b: &mut Bencher) {
        let range = 0..100000;
        let vec = range.collect::<Vec<i64>>();

        b.iter(|| into_iter(vec.clone()));
    }
}

結果:

> cargo bench
   Compiling iterator-bench v0.1.0
    Finished release [optimized] target(s) in 0.61 secs
     Running target/release/deps/iterator_bench-b01db65774dc9996

running 2 tests
test tests::bench_into_iter ... bench:      38,553 ns/iter (+/- 14,812)
test tests::bench_iter      ... bench:      39,507 ns/iter (+/- 20,207)

test result: ok. 0 passed; 0 failed; 0 ignored; 2 measured; 0 filtered out