達人の驚くほど小さいリファクタリング

Event:

リファクタリングLT

Presented:

2022/07/28 nikkie

お前、誰よ（自己紹介）

  • Python（とアニメ）大好き にっきー ／ Twitter @ftnext ／ GitHub @ftnext

  • 株式会社ユーザベースでアジャイル開発するデータサイエンティスト（自称えぬえるぴーや😎）

  • イチオシアニメ『アイの歌声を聴かせて』 ㊗️ 配信開始 ！

面白いからみんな観て🙏（配信も！）

ラクスさんのLT会、お世話になっております

ミノ駆動本_読書py 共催しています

達人の驚くほど小さいリファクタリングについて話します

ここから本題

このLTで伝えたいこと

  • 「リファクタリングのステップ、 小さっ！」という衝撃を共有

  • 1個1個は小さいリファクタリング、それを 何回も何回も行う ことでコードが良くなっていく！（と理解）

参考書籍

  • 元ネタは『The Art of Agile Development Second edition』（James Shore）の リファクタリングの章

    • 書籍で紹介されたリファクタリングをPythonに書き換えています（リファクタリング方針についてのフィードバックはJamesさん・Pythonのコードについてはnikkieへ）

  • Martin Fowler『リファクタリング』第1章にも「小さい！」という感想を抱きました

今回のサンプルコード

アルファベットを13文字入れ替える

>>> transform("a")  # 13文字先のnに変える（小文字のまま）
'n'
>>> transform("N")  # 13文字前のAに変える（大文字のまま）
'A'

変換表

a

b

c

d

e

f

g

h

i

j

k

l

m

n

o

p

q

r

s

t

u

v

w

x

y

z

 
>>> transform("NIKKIE")
'AVXXVR'
>>> transform("satomi")
'fngbzv'

アルファベットでないなら変換しません

>>> transform("satomi1231")  # 数字は変換されない
'fngbzv1231'
>>> transform("🤗")  # emojiも変換されない
'🤗'

リファクタリング前の実装

def transform(input_: str) -> str:
    if not isinstance(input_, str):
        raise TypeError("Expected string parameter")

    result = ""
    for character in input_:
        char_code = ord(character)
        result += transform_letter(char_code)
    return result


def transform_letter(char_code: int) -> str:
    if is_between(char_code, "a", "m") or is_between(char_code, "A", "M"):
        char_code += 13
    elif is_between(char_code, "n", "z") or is_between(char_code, "N", "Z"):
        char_code -= 13
    return chr(char_code)


def is_between(char_code: int, first_letter: str, last_letter: str) -> bool:
    return code_for(first_letter) <= char_code <= code_for(last_letter)


def code_for(letter: str) -> int:
    return ord(letter)

Pythonに書き直しました（Python 3.10）

変換戦略

  • 関数 transform は、受け取った文字列の1文字ごとに 文字コードに変換して transform_letter 関数を呼び出す

def transform(input_: str) -> str:
    if not isinstance(input_, str):
        raise TypeError("Expected string parameter")

    result = ""
    for character in input_:
        char_code = ord(character)
        result += transform_letter(char_code)
    return result

transform_letter 関数

  • 文字コードを受け取り、文字列を返す

  • A-Ma-m の場合と N-Zn-z の場合で分岐（判定に is_between 関数を使用）

def transform_letter(char_code: int) -> str:
    if is_between(char_code, "a", "m") or is_between(char_code, "A", "M"):
        char_code += 13
    elif is_between(char_code, "n", "z") or is_between(char_code, "N", "Z"):
        char_code -= 13
    return chr(char_code)

is_between 関数

  • 文字コードと範囲の開始・終了の文字を受け取る

  • code_for 関数で文字コードに変換して、範囲に含まれるか返す

def is_between(char_code: int, first_letter: str, last_letter: str) -> bool:
    return code_for(first_letter) <= char_code <= code_for(last_letter)

def code_for(letter: str) -> int:
    return ord(letter)

今回のリファクタリングのアイデア

文字コードにせずに 文字のまま でも比較できる！

>>> ord("a") <= ord("c") <= ord("m")
True
>>> "a" <= "c" <= "m"
True

リファクタリング後の実装

import re


def transform(input_: str) -> str:
    if not isinstance(input_, str):
        raise TypeError("Expected string parameter")

    return re.sub(
        r"[A-Za-z]",
        lambda matchobj: transform_letter(matchobj.group(0)),
        input_,
    )


def transform_letter(letter: str) -> str:
    rotation = 13 if letter.upper() <= "M" else -13
    return chr(ord(letter) + rotation)

すごーくスッキリ！✨

transform の単体テストコードはあります

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

https://github.com/ftnext/aoad2e-py/blob/start-refactoring/refactoring/test_rot13.py

質問：皆さんはどこからリファクタリングしますか❓

💡文字コードでなく文字で比較する

def transform(input_: str) -> str:
    if not isinstance(input_, str):
        raise TypeError("Expected string parameter")

    result = ""
    for character in input_:
        char_code = ord(character)
        result += transform_letter(char_code)
    return result


def transform_letter(char_code: int) -> str:
    if is_between(char_code, "a", "m") or is_between(char_code, "A", "M"):
        char_code += 13
    elif is_between(char_code, "n", "z") or is_between(char_code, "N", "Z"):
        char_code -= 13
    return chr(char_code)


def is_between(char_code: int, first_letter: str, last_letter: str) -> bool:
    return code_for(first_letter) <= char_code <= code_for(last_letter)


def code_for(letter: str) -> int:
    return ord(letter)

文字コードでなく文字で比較しよう（nikkieの場合）

  • transform_letter 関数をいじっていく

  • 引数 char_code 消して、 letter 導入

def transform_letter(char_code: int) -> str:
    if is_between(char_code, "a", "m") or is_between(char_code, "A", "M"):
        char_code += 13
    elif is_between(char_code, "n", "z") or is_between(char_code, "N", "Z"):
        char_code -= 13
    return chr(char_code)

文字コードでなく文字で比較しよう（nikkieの場合）

  • 導入したletter引数を渡す is_between 関数も変えて

  • あ、char_code どうするんだ？

  • 格闘の末、テスト全部通った！できた🙌

def transform_letter(letter: str) -> str:
    if is_between(letter, "a", "m") or is_between(letter, "A", "M"):
        char_code += 13
    elif is_between(letter, "n", "z") or is_between(letter, "N", "Z"):
        char_code -= 13
    return chr(char_code)

文字コードでなく文字で比較しよう（達人の場合）

  • まずは transform_letter 関数に 引数を追加するだけ

  • テストを全部通ることを確認（間違えていない😎）

-        result += transform_letter(char_code)
+        result += transform_letter(character, char_code)

-def transform_letter(char_code: int) -> str:
+def transform_letter(letter: str, char_code: int) -> str:

衝撃の「引数追加するだけ」

  • 2手目3手目を続ける前提で初手は引数追加だけ

  • 動きをいきなり変えない のか！！

  • 達人がどのようにリファクタリングするか一緒に見ていきましょう（10分に収まらない量なので、時間の許す限り）

リファクタリング with 達人

def transform(input_: str) -> str:
    if not isinstance(input_, str):
        raise TypeError("Expected string parameter")

    result = ""
    for character in input_:
        char_code = ord(character)
        result += transform_letter(char_code)
    return result


def transform_letter(char_code: int) -> str:
    if is_between(char_code, "a", "m") or is_between(char_code, "A", "M"):
        char_code += 13
    elif is_between(char_code, "n", "z") or is_between(char_code, "N", "Z"):
        char_code -= 13
    return chr(char_code)


def is_between(char_code: int, first_letter: str, last_letter: str) -> bool:
    return code_for(first_letter) <= char_code <= code_for(last_letter)


def code_for(letter: str) -> int:
    return ord(letter)

初手： transform_letter 関数に引数追加

-        result += transform_letter(char_code)
+        result += transform_letter(character, char_code)

-def transform_letter(char_code: int) -> str:
+def transform_letter(letter: str, char_code: int) -> str:

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

2手目： is_between 関数に引数追加

-    if is_between(char_code, "a", "m") or is_between(char_code, "A", "M"):
+    if is_between(letter, char_code, "a", "m") or is_between(letter, char_code, "A", "M"):

-    elif is_between(char_code, "n", "z") or is_between(char_code, "N", "Z"):
+    elif is_between(letter, char_code, "n", "z") or is_between(letter, char_code, "N", "Z"):

-def is_between(char_code: int, first_letter: str, last_letter: str) -> bool:
+def is_between(letter: str, char_code: int, first_letter: str, last_letter: str) -> bool:

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

3手目： 文字の比較 に実装を変更（ is_between 関数）

ついにリファクタリングのアイデアを実施

-    return code_for(first_letter) <= char_code <= code_for(last_letter)
+    return first_letter <= letter <= last_letter

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

4手目： 使わなくなった code_for 関数を削除

-def code_for(letter: str) -> int:
-    return ord(letter)

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

5手目： is_between 関数をインライン化

いまの is_between 関数の本体は 名前と同じくらい分かりやすい！

-    if is_between(letter, char_code, "a", "m") or is_between(letter, char_code, "A", "M"):
+    if ("a" <= letter <= "m") or ("A" <= letter <= "M"):
-    elif is_between(letter, char_code, "n", "z") or is_between(letter, char_code, "N", "Z"):
+    elif ("n" <= letter <= "z") or ("N" <= letter <= "Z"):

-def is_between(letter: str, char_code: int, first_letter: str, last_letter: str) -> bool:
-    return first_letter <= letter <= last_letter

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

6手目： transform_letter 関数で letter から char_code を作れる

引数 char_code を消す前のステップ

def transform_letter(letter: str, char_code: int) -> str:
+    char_code = ord(letter)

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

7手目： char_code 引数削除

-        char_code = ord(character)
-        result += transform_letter(character, char_code)
+        result += transform_letter(character)

-def transform_letter(letter: str, char_code: int) -> str:
+def transform_letter(letter: str) -> str:

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

この時点もだいぶスッキリ✨

def transform(input_: str) -> str:
    if not isinstance(input_, str):
        raise TypeError("Expected string parameter")

    result = ""
    for character in input_:
        result += transform_letter(character)
    return result


def transform_letter(letter: str) -> str:
    char_code = ord(letter)
    if ("a" <= letter <= "m") or ("A" <= letter <= "M"):
        char_code += 13
    elif ("n" <= letter <= "z") or ("N" <= letter <= "Z"):
        char_code -= 13
    return chr(char_code)

さらなるアイデア（おそらく途中で時間切れ）

  • 正規表現 r"[A-Za-z]" にマッチする文字だけ置き換える

  • Pythonでは re.sub に関数を渡す（transform_letter を渡す）

https://docs.python.org/ja/3/library/re.html#re.sub

re.sub(r"[A-Za-z]", マッチした各文字を変換する関数, 対象文字列)

正規表現導入

+import re

-    result = ""
-    for character in input_:
-        result += transform_letter(character)
-    return result
+    return re.sub(
+        r"[A-Za-z]",
+        lambda matchobj: transform_letter(matchobj.group(0)),
+        input_,
+    )

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

transform_letter のif文を単純に（WIP）

正規表現を使ったことで、 A-Za-zでのみ transform_letter 関数が呼び出される

-    if ("a" <= letter <= "m") or ("A" <= letter <= "M"):
+    if letter.upper() <= "M":

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

transform_letter のif文を単純に

-    elif ("n" <= letter <= "z") or ("N" <= letter <= "Z"):
+    else:

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

transform_letter の処理に rotation 変数導入

何文字だけ文字コードを変更するかを表す変数

     char_code = ord(letter)
     if letter.upper() <= "M":
         char_code += 13
+        rotation = 13
     else:
         char_code -= 13
+        rotation = -13

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

char_code 変数に再代入しない（ transform_letter 関数）

     char_code = ord(letter)
     if letter.upper() <= "M":
-        char_code += 13
         rotation = 13
     else:
-        char_code -= 13
         rotation = -13
-    return chr(char_code)
+    return chr(char_code + rotation)

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

char_code 変数のインライン化（ transform_letter 関数）

-    char_code = ord(letter)

-    return chr(char_code + rotation)
+    return chr(ord(letter) + rotation)

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

条件式を使って if 文を消す

Pythonに馴染みのない方へ、条件式は 三項演算子 だと思ってください

-    if letter.upper() <= "M":
-        rotation = 13
-    else:
-        rotation = -13
+    rotation = 13 if letter.upper() <= "M" else -13

Tests passed✅

$ python -m unittest
........
----------------------------------------------------------------------
Ran 8 tests in 0.001s

OK

小さいステップを積み重ねてリファクタリングできた！！🙌

import re


def transform(input_: str) -> str:
    if not isinstance(input_, str):
        raise TypeError("Expected string parameter")

    return re.sub(
        r"[A-Za-z]",
        lambda matchobj: transform_letter(matchobj.group(0)),
        input_,
    )


def transform_letter(letter: str) -> str:
    rotation = 13 if letter.upper() <= "M" else -13
    return chr(ord(letter) + rotation)

書籍を通じて達人とリファクタリングしてみて

まとめ🌯に代えて、気付きを最後に共有します（気付きについてのフィードバックはnikkieまで）

IMO: クソデカリファクタリングしかやってこなくて、すみませんでした！（懺悔）

  • リファクタリング＝ 小さいステップを積み上げる と認識が変わった

  • ステップが小さいので、テストが落ちているREDの時間が最小にできる

  • チームでどうやるかは実践して学びを得たい

IMO: 小さいステップでリファクタリングするには

  • 『リファクタリング』にあるような知識（例：関数のインライン化）が必要

  • テクニックを知っているからこそ 小さいステップに分解できる

  • テクニックを知る＝IDE操作を知るにもなりそう

IMO: 小さいステップのリファクタリングの効果

  • 小さいステップに分けているからこそ 今はここまで ができる

    • 出した例では正規表現を導入せずに止めてもいい

    • 今は時間がないけど、ここは設計変えていきたいから「 一手目として変数追加だけ やろう」もできるはず（👉 発表後記）

IMO: 『レガシーコード改善ガイド』を思い出す

  • 小さなステップに分けて、 いまできる手数だけ リファクタリング

  • betterな設計に向けてチームが動き出すための足がかりを作るということ🌱

  • 6章 スプラウトクラスやラップクラスに通じるかも

結びに：ケント・ベック言ってた！（『エクストリームプログラミング』 はじめに）

どんな状況でも必ず改善できる。
どんなときでもあなたから改善を始められる。
どんなときでも今日から改善を始められる。

達人のリファクタリングを知り、 小さく改善 していけるイメージ！

小さなリファクタリングを積み上げていきましょう！

ご清聴ありがとうございました

関連アウトプット

IMO 発表後記：一手目として変数追加だけ

  • チャットではネガティブな反応が多数だった（と思っている）こちらについて発表後版で補足します

  • なんにでもメリット・デメリットはあると思っていて、「一手目として変数追加だけ」も実践して学びを得ようと思っています

IMO 発表後記：一手目として変数追加だけ

  • 開発者のコミュニケーションのきっかけとして期待（コードを読んで「私ここやっておきますね」ができるのでは）

  • 数手重ねた小まとまり まで進めたほうが現実的なのかも

IMO 発表後記：一手目として変数追加だけ

  • ラップクラスでも「そんなことしたら前よりも悪くなる」という反応はあったと聞いています

  • 「その時点では、そのとおりです」と続きます（『レガシーコード改善ガイド』 p.85）

  • 小さなリファクタリングを 重ねる前提 で 足がかりを作る ためにやるのかなと理解しています

EOF