Help us understand the problem. What is going on with this article?

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

More than 3 years have passed since last update.

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

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

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


リファクタリングとは?

  • (できれば)振る舞いを変えずにコードを変えること

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

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

  • 一貫性を上げる
x = "foo"
y = 'bar'

 ->

x = "foo"
y = "bar"
  • DRY
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 (平静を保ち、普段の生活を続けよ)とは、イギリス政府が第二次世界大戦の直前に、開戦した場合のパニックや戦局が悪化した場合の混乱に備えて作成した、国民の士気を維持するための宣伝ポスターである(ウィキペディア)。

557971148_32ba6514a3_o.png

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

スクリーンショット 2015-09-08 8.26.28.png

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

  1. 開発者の幸せ
  2. パフォーマンスを上げる
  3. 将来の作業のための安心を得る
    • AKA 技術的負債を返済する(?AKA paying off technical debt)
  4. 開発者の教育

第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は新しいバージョンを出すとき、古いバージョンに戻ることができるようにしている

スクリーンショット 2015-09-08 10.03.12.png

  • 行数は増えてしまう

+80 -7

分解のまとめ

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

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

Backscatter

https://github.com/fspaolo/code/tree/master/backscatter

refactoring+-+yapc+20151.png

def data
  backscatter_trace 72
  load_data
end
  • メソッドが呼ばれるときのコールスタックを記録し分析する

refactoring+-+yapc+20152.png

refactoring+-+yapc+20153.png

refactoring+-+yapc+20154.png

Science

https://rubygems.org/gems/scientist

refactoring+-+yapc+20155.png

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
  • 継続的にフィーチャーごとの指標を記録している

refactoring+-+yapc+20156.png

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

refactoring+-+yapc+20157.png

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

refactoring+-+yapc+20159.png

スクリーンショット 2015-09-09 8.48.09.png

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

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

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

エディタ上の道具

  1. 検索・置換(?)
  2. リソースを展開(?)
  • エディタのレベルでのリファクタリング支援は物足りない

言語上の道具

  • 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 さんに教えて頂きました。

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

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

Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
Comments
Sign up for free and join this conversation.
If you already have a Qiita account
Why do not you register as a user and use Qiita more conveniently?
You need to log in to use this function. Qiita can be used more conveniently after logging in.
You seem to be reading articles frequently this month. Qiita can be used more conveniently after logging in.
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away