昨日、リファクタリングに関するLTをしたことさんの発表に関していろいろ思ったことがあったので、ちょっと書いておきたい。
nikkieさんのスライドはこちら：
発表概要
詳細はスライドを見てもらうとして、ざっくりいうと「リファクタリングする場合、驚くほど小さいステップで進めていった方がいいよ」という話。
その中で自分が「ちょっとこれはなぁ」と思ったのが、上記スライドの発表後記にも付け加えられてる通り「一手目として変数追加だけ」というもの。
nikkieさんのリファクタリング前としているコードは以下：
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)
ここで「is_between()では文字コードで大小比較してるけど、文字で直接大小比較できるよね」というのが最初のリファクタリングの発想で、その第一歩とされたのが次の変数追加：
- 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:
でも、これって未使用の変数を追加することになってて、それってよくないよって話をしたい。
やるなら中途半端な状態を作っちゃダメ。
未使用変数の問題
意図が分からない
未使用変数があると、それを読んだ人は「あれ？ この変数使ってないけど、いいのかな？」って引っかかりを覚える。
そういう疑問を浮かべてしまうコードは良くないコードで、いわゆる「コードの臭い」そのもの。
そういった「コードの臭い」を取り除くのがリファクタリングなのに、一時的とはいえ、それが埋め込まれたコミットが生じてしまうのは問題。
使うつもりで変数を増やしたのに間違って別の変数を使ってしまって増やした変数を使い忘れてるのか、それともあとで使うつもりだけど今は使ってないのか、あるいは他に同じインタフェースがあってそのインタフェースに合わせた結果として未使用変数が増えたのか、コミットからは分からない。
linterで警告される
上記のように、未使用変数は「間違って使ってないのか」「他の都合で未使用変数になっているのか」「一時的な変更のせいなのか」が分からないので、linterで静的解析を行うと警告が出る場合がある。
とくにC言語などのコンパイラ言語の場合、「定義されてない変数を使ってる」のはコンパイルエラーになるけど、「定義したけど使ってない変数」はコンパイルエラーにはならないので、linterでチェックすることが多い。
それくらい未使用変数っていうのは問題で、とくにコンパイルでエラーが検出できないので厄介な問題だったりする。
そういうのがあまり知られてない気がする。。。
なお、C言語の場合は「この変数は未使用で問題ない」ということを明示的に示す慣用的な記法もあって、それはvoidにキャストするということ。
こうするとlinterも「あぁ、この変数は使わないでOKなんだな」と理解して警告を出さなくなる。
たとえば、今回の例で最初の一手後のコードをC言語っぽく書くと次のようになる：
char transform_letter(char letter, int char_code) {
if (is_between(char_code, 'a', 'm') || is_between(char_code, 'A', 'M')) {
char_code += 13;
} else if (is_between(char_code, 'n', 'z') || is_between(char_code, 'N', 'Z')) {
char_code -= 13;
}
return (char)char_code;
}
このままlinterにかけると「letterが未使用変数だ」という警告が出る。
それを防ぐには、次のようにする必要がある：
char transform_letter(char letter, int char_code) {
(void)letter;
...
あるいは、マクロを用意しておいて次のようにしたりもする：
#define UNUSED(x) (void)(x)
char transform_letter(char letter, int char_code) {
UNUSED(letter);
...
Pythonの場合もFlake8で確認してみると、引数の未使用は警告されなかったけど（本来は警告されるべき）、ローカル変数で未使用のものは警告された。
なので、たとえば次のようなコードがあったとする：
def clip(value: int) -> int:
if value >= 10:
return 10
return value
この10がマジックナンバーなので、説明変数を導入しようと思ってまずは次のようにしたとする：
def clip(value: int) -> int:
max_value = 10
if value >= 10:
return 10
return value
この段階でFlake8でチェックすると、次のような警告が出る：
...: F841 local variable 'max_value' is assigned to but never used
もちろん、次のように直せばこの警告は消える：
def clip(value: int) -> int:
max_value = 10
if value >= max_value:
return max_value
return value
つまり、未使用変数を導入しただけだとむしろコードは悪化してて、それをちゃんと使うところまでやらないとダメっていうこと。
今回のリファクタリングも都度テストを回して「OK」ってやってたけど、静的解析まで考えると「NG」に近い状況が発生してる
（残念ながら引数の未使用はFlake8だとチェックされないっぽいけど）。
オブジェクト指向でもそうなんだけど、とにかく不正な状態を作っちゃダメで、もし作らざるを得ないならコメントとかが必須。
関数の戻り値の未使用
ちょっと関連する話で、関数の戻り値を使用してない場合にもC言語のlinterは警告を出す。
こういうときにもその値を使わないことを示すのにはvoidキャストが使われる。
PythonのFlake8だとこれはチェックしてくれないみたいだけど、安全側に倒すなら本当はチェックした方がいい。
たとえば、次のようなミス、やったことある人もいるのでは？
a = [10, 8, 15]
sorted(a)
print(a)
これはsorted()はソートしたあとのリストを返すんだけど、元のリストをソートしてくれるわけではないので、こういうことが起きる。
ここで関数の戻り値が未使用という警告が出ていたのなら、次のように直すことができる：
a = sorted(a)
ただ、こうすると今度は使わない関数の戻り値まで警告が出ることになる。
そういった使わない戻り値を明示する方法をPythonはちゃんと持ってて、_ に代入するといい。
_ = some_func(a, b)
なので、未使用の引数も本当は_に代入して未使用ということを明示した方がいいのかもしれない：
def transform_letter(letter: str, char_code: int) -> str:
_ = letter
...
あるいは、ちょっと意図が分かりにくいのでunused()みたいな何もしないメソッドを用意するとか：
def unused(arg: Any) -> None:
_ = arg
def transform_letter(letter: str, char_code: int) -> str:
unused(letter)
...
そんな感じで、未使用変数一つとっても実はいろいろ議論するべき内容がある。
それくらい未使用変数を作るのは問題なんだけど、まぁ普段は未使用変数なんて作ることもないだろうから、それが問題でlinterでのチェック対象になってることや、未使用でもOKだということを明示する方法が知られてない気がする。
今日はここまで！
