More than 5 years have passed since last update.

GitHubエンジニアによる「リファクタリングにおける冒険とは」の翻訳

Last updated at 2015-09-17Posted at 2015-09-09

GitHubのエンジニア Ben Lavender によるYAPC2015のセッション「Adventures in Refactoring」のスライドが公開されたので、翻訳を試みました。

注）私はセッションには行っていないため、いくつかわからない箇所がありますので、編集リクエストを送って頂けると幸いです（同時通訳の内容を公開してくれたら、もう少しわかるのですが・・・）。

ちなみに英語ですが動画も公開されています。

Adventures in Refactoring / Ben Lavender

リファクタリングとは？

（できれば）振る舞いを変えずにコードを変えること

第1部リファクタリングする理由

リファクタリングする理由として悪いもの

一貫性を上げる

x = "foo"
y = 'bar'

 ->

x = "foo"
y = "bar"

user = load_user
widgets = user.widgets
user = load_user
widgets = user.widgets

 ->

load_user_widgets

抽象化

user.get_first_address_city

 ->

user.addresses.first.city

これらは、やるのはいいことだが、これをやるためにリファクタするというのは良くない

なぜ良くないのか？

良いソフトウェア開発では、改善を計測する
ではリファクタリングの改善を計測してみよう

どの改善を計測するか？

1. コードの正しさ

コードの正しさは維持されているか？

user.get_first_address_city

 ->

user.addresses.first.city

ダメ

user.get_first_address_city
# "Toledo"

 ->

user.addresses.first.city
# "Akron"

2. 行数（重要）

行数を減らす
少ないコードのバグは少ない
だから、コードを減らすことは重要
ダメなコミット

+181 -130

+31 -40

こういうコミットには賞賛を込めて🔪（包丁:hocho:）マークを付けてあげよう

3. テストカバレッジ

テストカバレッジを上げる
（以下のコードがよくわからないです。誰か教えて下さい。）

# 100% test coverage!
if foo
  puts "foo!"
end
if bar
  explode if foo
  puts "bar!"
end

4. テストしやすさ

テストしやすくする
テストしにくい例

describe "deployment" do
  context "when master needs merging" do
  context "when CI is green" do
  # test
  context "when CI is not green" do
  # test
  context "when branch is up-to-date" do
  context "when CI is green" do
  # test
  context "when CI is not green" do
  # test

変わっている部分だけをテストできるように、ネストしたコンテキスト（ここではMasterMerged、CIGreen）をサブクラスに分離する

class ChecklistItem
  ...
end
class MasterMerged < ChecklistItem
  ...
end
class CIGreen < ChecklistItem
  ...
end

デプロイクラスで #run_checklist メソッドを呼ぶとチェックリストをチェックする（？）

class Deployment
  def run_checklist!
    checklist.each do |checklist_item|
    # checklist_itemは
    # MasterMergedのようなクラス
    checklist_item.new(self).check

対象（MasterMergedコンテキスト、CIGreenコンテキスト）のテストを書きやすい

describe MasterMerged do
  it "merges master if required"
  it "does nothing if master merge not required"
describe CIGreen do
  it "adds a blocker if CI is not green"
  it "lets the deployment continue if CI is green"

5. パフォーマンス

パフォーマンスを上げる

6. 開発者の幸福度😁

Keep Calm and Carry On （平静を保ち、普段の生活を続けよ）とは、イギリス政府が第二次世界大戦の直前に、開戦した場合のパニックや戦局が悪化した場合の混乱に備えて作成した、国民の士気を維持するための宣伝ポスターである（ウィキペディア）。

これををもじり、「冷静に、改善指標に集中せよ」

第1部まとめ：リファクタリングする理由とは？

開発者の幸せ
パフォーマンスを上げる
将来の作業のための安心を得る
- AKA 技術的負債を返済する（？AKA paying off technical debt）
開発者の教育

第2部リファクタリングのコツ

コツ1 少しずつ変更して、少しずつ幸せになる

if foo
  if bar
    if fum
      puts "yeah!"
    end
  end
end

 ->

return unless foo && bar && fum
puts "yeah!"

user = load_user
widgets = user.widgets
user = load_user
widgets = user.widgets

 ->

load_user_widgets

user.get_first_address_city

 ->

user.addresses.first.city

x = "foo"
y = 'bar'

 ->

x = "foo"
y = "bar"

if some_pretty_long_condition ?
  "foo string" :
  "bar string"

 ->

return "foo string" if some_pretty_long_condition
"bar string"

object.some_method "foo", :fi_fo_fum => widget, :baz
=> fum, :free => fallin

 ->

object.some_method "foo", 
  :fi_fo_fum => widget,
  :baz => fum,
  :free => fallin

コーディングのスタイルガイドを入手する

$ go fmt

これはGo言語のコマンドで、フォーマットした内容を上書きしつつ、フォーマットして差異のあったファイル名を標準出力に表示する。

コツ2 動詞に型をつける

よくあるクラス

class Account                  # 口座
  def transfer(                # 送金する
      amount, other_account)
    other_account.add(amount)  # 残額を増やす
    self.subtract(amount)      # 残額を減らす
  end
end

別ジョブ処理を追加

class Account
  def transfer(amount, other_account, at)
    # 別のジョブが動いていたら何もしない
    if at
      BackgroundJob.schedule(at, self,
        :transfer, amount,
        other_account)
      return
    end
  other_account.add(amount)
  self.subtract(amount)
  end
end

「送金する」クラスをつくると・・・

def Transaction
  # 送金元、送金先、送金額、時刻をとってインスタンス化
  def initialize(@from, @to, @amount, @at)
    BackgroundJob.schedule(self, @at)
  end
  # 送金を実行する
  def run(from, to, amount)
    from.subtract(amount)
    to.add(amount)
  end
end

イイ感じ

class Account
  def transfer(amount, other_account, at)
    Transaction.new(
      self, other_account, amount, at)
  end
end

こんな拡張ができる

transaction.successful?
transaction.finished_at
transaction.failure_reason

コツ3 便利な中間層をつくる

このコードを・・・

pull.branch_valid?
pull.branch_exists?

こうする

pull.branch.valid?
pull.branch.exists?

このプロパティ

pull.git_mergeable?
pull.status_mergeable?

このように使うのだが・・・

if pull.git_mergeable?
  show_merge_button
else
  show_disabled_merge_button
end

if pull.stable?
 show_merge_button
else
 show_disabled_merge_button
end

こうする

pull.merge_status.okay?
pull.merge_status.git_okay?
pull.merge_status.ci_okay?

すると例えばこんなクラスが・・・

class PullRequest
  def git_mergeable?
  end
  def status_mergeable?
  end
  def stable?
    git_mergeable? && status_mergeable?
  end
end

こう書ける

class MergeState
  def initialize(pull)
   @pull = pull
  end
  def git_okay?
  end
  def ci_okay?
  end
  def stable?
    git_okay? && ci_okay?
  end
end

コツ4 使われていない中間層を取り除く

例えばこんなクラス

class CampfireAdapter < ChatAdapter
  #...
end
class ChatAdapter
  def post(room, message)  # ルームに投稿する
    room = lookup_room(room)
    room.post(message)
  end
end

こう使うのだが・・・

chat = ChatAdapter.new(:campfire)
chat.post(room, message)  # ルームに投稿する

クラスをこう書けば・・・

class Campfire          # ChatAdapterクラスは廃止
  def self.post(        # ルームに投稿する
      room_id, message)
    RestClient.post     # RESTリクエストを送る
      "rooms/room_id",
      {:message => message }
  end
end

こう使うことができる

Campfire.post room, message

行数をグッと減らすことができる

+87 -422

問題

1. 既存のバグ

リファクタリング中に、いままでテストされていなかったバグを見つける場合がある
リファクタリング中にバグを直してはいけない
作業が混乱してしまう

2. パフォーマンスの変化

抽象化レイヤーを増やすリファクタリングは、速度が許容できる場合に限る

3. コンテキスト

#fetchに引数を追加したいとする

gitrpc.client do |c|
  c.fetch(file)
end

 ->

gitrpc.client do |c|
  c.fetch(file, 4096)
end

以下のように#fetchからentry.dataまでのコールスタックが長い場合、修正が難しい

class TreeEntry
  def data
    ...
  end
end

fetch(file)
find_file_by_name(file)
setup_gitrpc_client(...)
rpc_client_connect(...)
entry = rpc_get_current_tree(...)
entry.data

こういう場合は#fetchをdeprecateして新メソッドにする

module GitRPC
  def head_bytes(file)
    fetch(file).head_bytes
  end
  def tail_bytes(file)
    fetch(file).tail_bytes
  end
end

 ->

module GitRPC
  def head_bytes(file)
    fetch(file, 4096).head_bytes
  end
  def tail_bytes(file)
    fetch(file).tail_bytes
  end
end

以上のようにリファクタリングは危険を伴う
特に大きなリファクタリング
そこで・・・

コツ5 大きなリファクタリングは小さなリファクタリングに分解する

以下は分解のコツ

分解のコツ1 廃止したことを明示する

deprecateを使う
何を何に変えたのか、メッセージを書いておこう

module GitRPC
  # deprecated
  def fetch(file)
  end
  def fetch_with_limit(file, size)
  end
end

module GitRPC
  def fetch(file)
  end
  deprecate :fetch, 
    "Migrate to fetch_with_limit plz"
  def fetch_with_limit(file, size)
  end
end

module GitRPC
  def head_bytes(file)
    fetch(file).head_bytes
  end
  def tail_bytes(file)
    fetch(file).tail_bytes
  end
end

module GitRPC
  def fetch(file)
  end
  deprecate :fetch, 
    "Migrate to fetch_with_limit plz"
  def tail_bytes(file)
    # Not deprecated here, really
    fetch(file).tail_bytes
  end
end

分解のコツ2 後方互換をとる

（このDBの出力のようなものはいったい？）

--------------
id | current |
--------------
1  | false   |
2  | true    |
--------------

--------------
id | current |
--------------
1  | true    |
2  | true    |
--------------

current_widgetが複数になり得るとき、current_widgetメソッドは常にひとつだけを返すように残しておき（後方互換）、current_widgetsメソッドを新設する

class Widget
  def current_widget
    find_by(:current => true)
  end

 ->

  def current_widget
    # 単数形なので.lastを付ける
    find_all(:current => true).last
  end
end

<%# current_widgetを使う %>
<div class="widget">
  <%= current_widget.name %>
</div>

 ->

<%# current_widgetsを使う %>
<ul class="widgets">
  <% current_widgets.each do |widget| %>
  <li><%= widget.name %></li>
  <% end %>
</ul>

Gmailは新しいバージョンを出すとき、古いバージョンに戻ることができるようにしている

行数は増えてしまう

+80 -7

分解のまとめ

大きなリファクタリングは危険である
危険を回避するには、ツールを作る

リファクタリングのためのツール

Backscatter

def data
  backscatter_trace 72
  load_data
end

メソッドが呼ばれるときのコールスタックを記録し分析する

Science

def get_widgets
  widgetify
end

 ->

def get_widgets
  widgetify
end
# 以下を追加
def get_widgets_new
  load_widgets_from_database
end

古いメソッドと新しいメソッドの2つを実行できる
古いメソッドは例外も含めて実行される
新しいメソッドは例外などを無視する形で実行される
結果をシステム挙動をこわさずに比較できる

def get_widgets
  science "widgets.loading" do |e|
  e.use { get_widgets_old }
  e.try { get_widgets_new }
  end
end

GitHub.subscribe "science.mismatch" do |payload|
  puts "old value returned #{payload.control}"
  puts "new value returned #{payload.candidate}"
end

継続的にフィーチャーごとの指標を記録している

正しく動いているかどうかをグラフ表示してくれる

パフォーマンステストができる

Gitのパーミッション絡みのリファクタリングに2年かかった
ABテストに似たような考えで、新しいメソッドと古いメソッドの両方をコードに組み込みリファクタリングした
複雑なリファクタではあるが、1年半状況を確認しながら安全に実施した。

第3部リファクタリングのイマイチなところ

Backscatterは時間がかかる
scienceはrubyしかない

エディタ上の道具

検索・置換（？）
リソースを展開（？）

エディタのレベルでのリファクタリング支援は物足りない

言語上の道具

Javaの@deprecatedアノテーション
これは大変良い

/**
 * @deprecated  As of release 1.3, replaced by {@link #getPreferredSize()}
 */
@Deprecated public Dimension preferredSize() {
  return getPreferredSize();
}

Cの廃止プラグマ

[[deprecated("Replaced by bar")]]
void foo(int);

C++14も入ってる
Djangoなどのフレームワークにもある

コンパイル時にわかる廃止は本当にやめて欲しい

（この例はいったい？）

$x = 1 | 2
x == 1
# true
x == 2
# true
$x = $x * 3
x == 3 && x == 6
# true

例：ありません！（？）

質疑応答

レガシーコードで，テストやドキュメントも明確な正解がわからない時どうするか？

ちゃんとカバレッジが高くなることは困難だが，出来る限り多くの正しいテストをきちんと追加しすることからはじめ，リファクタリングとはわけて考える
テストを描き切ってから、リファクタリングをする。別々に切り分ける。変更を行う前に6か8つのテストを加えるのは最初のプルリク。

Githubではリファクタリングの方針の意見が分かれたらどうしているか？

リファクタリングのアプローチに意見が分かれたらPull requestを使う
スタイルガイドがあることが大事で、セミコロンやクォーテーションはどっちがいいかはスタイルガイドを見ていく
テストの結果を見て、どこが改善された点か確認していいく
コミュニケーションが大事

リファクタリングと機能追加のどちらを優先するか？

機能追加に自信がないならリファクタリングを優先する

リファクタリングにデザインパターンを使っているか？

No
デザインパターン主導は良くない
なぜなら、テストのためにコードを書くことになってしまいがちだから
良いテストは、良いコードの結果である

リファクタリングは退屈でつまらなくないか？

素晴らしい仕事をしたら褒めよう
リファクタリングでもLGTM的なやつが大事
ハッピーな絵文字をたくさん使おう

テストもドキュメントもない場合のリファクタリングはどうしたら良いか？

まずテスト書いてカバレッジを上げる
そのあとにリファクタリングする

謝辞

スライドがアップロードされたことは@Minato128 さんに教えて頂きました。

以下のサイトを参考にさせて頂きました。

ありがとうございました。

334

330

You get articles that match your needs
You can efficiently read back useful information
You can use dark theme

What you can do with signing up