はじめに
Twitterを見てたら、気になる雑誌の特集を見つけました。
WEB+DB PRESS Vol.99の「Rubyで学ぶ！良いコードって何だろう？」という特集記事です。
WEB+DB PRESS Vol.99
- 作者: ?橋健一,谷口禎英,井本大登,山崎勝平,大和田純,内村元樹,坂東昌哉,平田敏之,牧大輔,板敷康洋,大?浩崇,穴井宏幸,原口宗悟,久田真寛,ふしはらかん,のざきひろふみ,うらがみ,ひげぽん,池田拓司,はまちや2,竹原,片田雄樹,渋江一晃,WEB+DB PRESS編集部編
- 出版社/メーカー: 技術評論社
- 発売日: 2017/06/24
- メディア: 大型本
- この商品を含むブログを見る
Ruby大好き！きれいなコード大好き！！な僕にとっては、この特集は読まずにはいられません！
早速買って読んでみました。
お～、なるほど、たしかにいいことが書いてある！
うんうん、そうそう・・・あれ？このコード、どうなん？
うーん、この例はちょっと微妙かも。。
と思ったところがちーらほら。
重箱の隅を突っつくような話をするのは野暮だなあとは思いつつ、それでも「良いコード」をうたう特集記事なのであれば、隅から隅まで「良いコード」を貫き通してほしい！
というわけで、このエントリではこの特集記事を読んでいて、ちょっと気になったサンプルコードをピックアップしていこうと思います。
本編へ進む前に
執筆者のみなさまへ
せっかくがんばって書いた記事に、あれこれいちゃもん付けてすいません！
先に謝っておきます。🙇🙇🙇
指摘事項の中には「そんな細かいところまでチェックしなくても」と思われるものもあるかもしれませんが、せっかくのいい特集なので、サンプルコードは徹底的にブラッシュアップした方がいいんじゃないかと思い、このエントリを書きました。
まだWEB+DB PRESS Vol.99を読んでいないみなさんへ
僕のエントリだけ読むと、「この記事、問題だらけなん？」って思われるかもしれませんが、そうではありません！
記事自体は非常に良い内容になっていますし、僕もほとんどの内容に同意しています。
また、説明の文章が非常に読みやすいのも、この記事のいいところだと思います。
ですので、良いコードを書けるようになりたいと思っているプログラマのみなさんはWEB+DB PRESS Vol.99を購入してもらっても損はありません。
安心して購入してください！
とはいえ、真剣に僕基準でサンプルコードをレビューしていくと、それなりの件数の改善ポイントが見つかりました。
僕はソニックガーデンの社内でも「一番コードにうるさくて、面倒くさい男」と思われてるそうなので（苦笑）、僕が仕事でコードレビューするなら、たぶんこれぐらいツッコミを入れます。
WEB+DB PRESS編集部のみなさんへ
このエントリが引用の範囲を超えていると判断された場合はエントリを削除しますので、ご連絡ください。
サンプルコードについて
WEB+DB PRESS内で使われているサンプルコードはGitHubでも公開されています。
お待たせしました。
それでは以下が本編です。
P12: 何もしないelseは必要か？
記事の一番最初に出てきたこのコード例ですが、いきなり「すごく微妙なライン」を持ってきたな～という印象です。
case hour
when 9
say_hello()
when 18
say_goodbye()
end
case hour
when 9
say_hello()
when 18
say_goodbye()
else
end
記事によると、「上のコード では、hour が 9 と 18 以外の場合に何もしないのは意図したことなのか不安が残るのに対し、下のコードでは何もしなくてよいという書き手の意図が読み手にもしっかりと伝わります。」とのことですが、この「何もしないelse」が必須かと言われると、うーん。。。
僕だったら「何もしないelse」は作らないですね。
このコード例であれば「elseがない＝9時と18時以外は何もしない」と解釈します。
もちろん「elseがないとコードを読んだ人が明らかに不安に感じる」という特殊なケースでは「何もしないelse」を書くことも検討しますが、このコード例のレベルであれば僕はelseは書きません。
（2017.6.30 13:30追記）
あえて何もしない条件分岐を作る場合、こういうコードならまだアリかな～と思います。
def greeting(hour)
case hour
when 9
'Hello'
when 18
'Good-bye'
when 0..23
else
raise ArgumentError, "Invalid hour: #{hour}"
end
end
greeting(0)
greeting(9)
greeting(18)
greeting(23)
greeting(24)
ここでは「予期しない時間（hour）」が渡ってきたときに、問題にすぐ気づけるようエラーを発生させています。（else節）
ただし、 "when 18" の直後に else を持ってくると0時や23時もエラーになってしまうので、 "when 0..23" の条件（hourが0～23なら真になる条件）を入れています。
ですが、特に何かをするわけではないので、 when節の中身は空っぽです。
こういうコードであれば、何もしない条件分岐を用いることで「hour が 9 と 18 以外の場合に何もしない」という意図が、よりハッキリするんじゃないかと思います。
（2017.7.3 12:00追記）
あと、「何もしないelse」を書く代わりにテストコードを書いておくのも有効だと思います。
そうすれば「開発者はこの条件だけを処理の対象にしたいと考えている」という意図がよりわかると思うので。
def greeting(hour)
case hour
when 9
'Hello'
when 18
'Good-bye'
end
end
require 'minitest/autorun'
class SampleTest < Minitest::Test
def test_greeting
assert_nil greeting(0)
assert_equal 'Hello', greeting(9)
assert_equal 'Good-bye', greeting(18)
assert_nil greeting(23)
end
end
P14: 設計の意図が全く読めないサンプルコード
続いてはこちらのコード例です。
class Foo
attr_accessor :bar
def print(content)
@bar + content
end
end
foo = Foo.new
foo.bar = "WEB+DB PRESS Vol."
method1(foo)
method2(foo)
method3(foo)
foo.print("99")
このコードは変数のスコープが広すぎる悪い例として挙げられているものです。
記事によると、「どうやらどこかで bar に nil が入ってしまっ たようです。（中略） そもそもこの bar は何度も代入したいものなのでしょうか。初期化時だけでは駄目なのでしょうか。」とのことなんですが、「そもそもこの bar は何度も代入したいものなのでしょうか。初期化時だけでは駄目なのでしょうか。」という問いに対する答えは、「要件による」としか言いようがありません。
しかし、このサンプルコードでは、Fooクラスと変数barの役割が全くわかりません。
また、method_1、method_2、method_3も何をするメソッドか全くわかりません。
いい名前が付いているメソッドであれば「barがnilに変わるケースもありそう」と思えるかもしれません。
もちろん、言わんとしていることはわかります。
「不用意にオブジェクトの状態を変更できるようにすべきではない」「無用な副作用をメソッドに持たせてはいけない」というのは、良いコードの大事な条件です。
ですが、ここでは何をやろうとしているのかわからないコード例になっているため、いまいち説得力に欠けているのが残念なポイントです。
ちなみに、まったく同じテーマではないですが、少し似た話をその昔Qiitaに書いたことがあります。
参考： （あなたの周りでも見かけるかもしれない）インスタンス変数の間違った使い方 - Qiita
P17: initメソッド？initializeメソッド？
次はマジックナンバーではなく、定数を使おうというこのサンプルコードです。
class Tweet
MAX_TWEET_LENGTH = 140
def init(content)
@content = content
end
def post!
if @content.length <= MAX_TWEET_LENGTH
else
end
end
end
定数を使うのはOKなのですが、initメソッドはいったい何なのでしょう？
initializeメソッド（JavaやC#でいうところのコンストラクタ）とは別のメソッドなのでしょうか？
このコードを見る限り、initializeメソッドでよいと思います。
そうしないと、newしたあとに、わざわざinitメソッドを呼び出さなければならないからです。
あえてのinitメソッドなのか、initializeメソッドと書くつもりだったのか、意図がつかめずモヤモヤします。
P18: 定数はドットでは参照できない（エラーが起きる）
さて、次はこちらのコードです。
class ExceptionCode
INCORRECT_CREDITCARD = 100
COMMUNICATION_FAILURE = 101
end
begin
paid!
rescue ServiceException => ex
if ex.code == ExceptionCode.INCORRECT_CREDITCARD
show_error "クレジットカードの情報が誤っています。"
redirect user_creditcard_path
elsif ex.code == ExceptionCode.COMMUNICATION_FAILURE
retry!
end
end
えーと、 "ExceptionCode.INCORRECT_CREDITCARD" ではなく、 "ExceptionCode::INCORRECT_CREDITCARD" （ドットではなくダブルコロンで区切る）が正ですね。
ドットを使うなら INCORRECT_CREDITCARD がメソッドになっていなければなりません。
しかし、 INCORRECT_CREDITCARD は定数なのでエラーが起きます。
irb(main):005:0> ExceptionCode.INCORRECT_CREDITCARD
NoMethodError: undefined method `INCORRECT_CREDITCARD' for ExceptionCode:Class
from (irb):5
from /Users/jit/.rbenv/versions/2.4.1/bin/irb:11:in `<main>'
ダブルコロンなら問題ありません。
irb(main):006:0> ExceptionCode::INCORRECT_CREDITCARD
=> 100
うっかりミスだとは思いますが、このへんはきちっとコードレビューしてほしかったな～と思うところです。
P18: クラスよりもモジュール？ / 例外クラスに定数を定義すべき？（状況による）
先ほどと同じこちらのコードです。
class ExceptionCode
INCORRECT_CREDITCARD = 100
COMMUNICATION_FAILURE = 101
end
もし、これが定数しか持たないクラスなのであれば、クラスではなくモジュールとして定義する方が良いでしょう。
なぜならExceptionCodeはインスタンス化する必要がないからです。
module ExceptionCode
INCORRECT_CREDITCARD = 100
COMMUNICATION_FAILURE = 101
end
ただし、これはあくまで短いサンプルコードですが、実務レベルのコードではインスタンス化する必要が出てくるかもしれません。
クラスにすべきか、モジュールにすべきかは、そのときの状況によります。
また、そもそもの話になりますが、 "ex.code == ExceptionCode::INCORRECT_CREDITCARD" のようにしてエラーコードを比較するのであれば、ServiceExceptionクラスでこれらの定数を定義している方が設計として自然だと思います。（これもまた状況によりますが）
begin
paid!
rescue ServiceException => ex
if ex.code == ServiceException::INCORRECT_CREDITCARD
show_error "クレジットカードの情報が誤っています。"
redirect user_creditcard_path
elsif ex.code == ServiceException::COMMUNICATION_FAILURE
retry!
end
end
P18: Rubyの例外クラス名は慣習的に～Errorで終わることが多い
またまた先ほどのコードを登場させます。
class ExceptionCode
INCORRECT_CREDITCARD = 100
COMMUNICATION_FAILURE = 101
end
begin
paid!
rescue ServiceException => ex
if ex.code == ExceptionCode.INCORRECT_CREDITCARD
show_error "クレジットカードの情報が誤っています。"
redirect user_creditcard_path
elsif ex.code == ExceptionCode.COMMUNICATION_FAILURE
retry!
end
end
rescue 節にServiceExceptionという例外クラスが登場していますが、Rubyの実行時例外は通常、StandardErrorクラスを継承します。
そしてStandardErrorクラスを継承した場合は、例外クラスの名前も～Errorで終わることが多いです。
参考: library _builtin (Ruby 2.4.0)
なので、ServiceException よりも ServiceError の方がRubyっぽい例外クラス名だと言えます。
ただし、これは慣習的な話なので、必ずそうすべきというルールではありません。
P18: retry! メソッドとは？
もう一度、先ほどのコードに登場してもらいます（4回目）。
class ExceptionCode
INCORRECT_CREDITCARD = 100
COMMUNICATION_FAILURE = 101
end
begin
paid!
rescue ServiceException => ex
if ex.code == ExceptionCode.INCORRECT_CREDITCARD
show_error "クレジットカードの情報が誤っています。"
redirect user_creditcard_path
elsif ex.code == ExceptionCode.COMMUNICATION_FAILURE
retry!
end
end
下から3行目に登場するretry!はどこで定義されているメソッドなんでしょうか？
Rubyにはretryを使って、例外処理をやり直したりすることができますが、retry! というビックリマーク付きの制御構造やメソッドは標準では存在しません。
もしかすると、retry! はどこかで定義されている独自のメソッドなのかもしれませんが、例外処理の中でそのようなメソッドを使うとRuby標準のretryと混同しやすいので、あまり良いメソッド名だとは言えません。
あくまで予想ですが、これは本来retryと書くつもりだったんじゃないかと思います。
P18: redirectメソッド？？
またまた先ほどのコードを出します。（これで最後です）
begin
paid!
rescue ServiceException => ex
if ex.code == ExceptionCode.INCORRECT_CREDITCARD
show_error "クレジットカードの情報が誤っています。"
redirect user_creditcard_path
elsif ex.code == ExceptionCode.COMMUNICATION_FAILURE
retry!
end
end
if文の中に出てくる "redirect user_creditcard_path" ですが、これがもしRailsのコードだとしたら、redirectではなく、redirect_toが正です。
ただし、記事の中にはどこにも「Railsのコードである」との言明はないため、架空のフレームワークに定義された架空のメソッドである可能性もあります。
とはいえ、user_creditcard_pathというメソッドもRailsっぽいです。
もし、これがRailsっぽくてRailsでないコードなのであれば、読者を困惑させる原因になります。
なので、「これはRailsのコードです」もしくは「これはあくまでRailsではない架空のWebアプリケーションです」との言明が本文中にほしかったなと思います。
P20: order.new の o が小文字
P20に出てくる以下のコードは、状況から見て "order.new" ではなく、 "Order.new" が正ですね。
（特殊な状況では小文字もありえるが、普通はまずない）
def product_paid_and_send_mail(user, product)
payment = Payment.new(product.price)
if payment.pay
order = order.new(user.id, product.id)
order.save
mail = Mail.new do
from "from@example.co.jp"
to user.mail
subject "商品を購入しました"
body "#{product.name}を購入しました"
end
mail.deliver
end
end
うっかりミスだと思いますが、コードレビューをしっかり！！
P20: 使われないローカル変数を登場させない
以下のコードの上から3行目、create_orderの戻り値をorderという変数に格納していますが、このorderは使われないローカル変数です。
良いコード例として紹介するのであれば、使われないローカル変数は登場させない方がいいと思います。
def product_purchase(user, product)
if pay_for(product)
order = create_order user.id, product.id
send_ordered_mail product.name, user.mail
end
end
def create_order(user_id, product_id)
order = Order.new(user_id, product_id)
order.save
end
P21: メソッドの名前は「動詞＋目的語」の方が自然
もう一度先ほどのコードを登場させます。
def product_purchase(user, product)
if pay_for(product)
order = create_order user.id, product.id
send_ordered_mail product.name, user.mail
end
end
ここでは "product_purchase" というメソッド名になっていますが、「商品を購入する」ためのメソッドなのであれば、 "purchase_product" のように「動詞＋目的語」の語順にした方がメソッド名として自然です。
参考： モデルやメソッドに名前を付けるときは英語の品詞に気をつけよう - Qiita
P26: インデントが1文字深い
下記コードの3行目、インデントが1文字ぶん深いです。
（細かいけどしっかりチェックするよ！！）
class Barista
def brew(drink)
drink.brew
end
end
P31: クラスは文字列にしなくてもwhen節に直接クラスを指定できる
以下はクラス名に依存する悪いコード例として出てきたものなので、ツッコむのも野暮かもしれませんが、気になったので一応書いておきます。
class Customer
def pay(payment_method)
case payment_method.class.to_s
when 'CreditCard'
payment_method.pay
when 'Bank', 'Conveni', 'PostOffice'
payment_method.transfer
else
raise ArgumentError.new('unknown payment method')
end
end
end
Rubyではwhen節に直接クラスを指定できるので、上のコードは次のように書いた方がシンプルです。
class Customer
def pay(payment_method)
case payment_method
when CreditCard
payment_method.pay
when Bank, Conveni, PostOffice
payment_method.transfer
else
raise ArgumentError.new('unknown payment method')
end
end
end
ただし、「もしかすると」ですが、この言語仕様はRubyの中でも比較的マイナーな部類に入る（と思う）ので、Rubyに詳しくない人でもわかるようにあえて文字列で比較したのかな？という気がしなくもありません。
P31: Conveniクラスがない
先ほど出てきたコードをもう一度登場させます。
class Customer
def pay(payment_method)
case payment_method.class.to_s
when 'CreditCard'
payment_method.pay
when 'Bank', 'Conveni', 'PostOffice'
payment_method.transfer
else
raise ArgumentError.new('unknown payment method')
end
end
end
class CreditCard end
class Bank end
class ConvenienceStore end
class PostOffice end
customer = Customer.new
customer.pay(CreditCard.new)
when節に "Conveni" と書いてありますが、その下に出てくるクラスは "ConvenienceStore" クラスです。
クラス名が異なるので、意図した通りに動きません。
このコードはあくまで「悪いコード例」ですが、僕がここで指摘した内容は本文中に「悪い部分」として取り上げられていないので、おそらく純粋なコーディングミスだと予想します。
P31: 例外を発生させるならデバッグしやすい情報を含めるべき
またまた先ほどの「悪いコード例」です。
class Customer
def pay(payment_method)
case payment_method.class.to_s
when 'CreditCard'
payment_method.pay
when 'Bank', 'Conveni', 'PostOffice'
payment_method.transfer
else
raise ArgumentError.new('unknown payment method')
end
end
end
想定外の選択肢（つまりelse節）に入ってきたら例外を発生させる、という考え方は良いのですが、どんなオブジェクトがやってきたのかわからないと、例外が起きた後のデバッグに苦労します。
なので、例外メッセージには次のようにpayment_methodのクラス名も含めた方が良いでしょう。
raise ArgumentError.new("unknown payment method: #{payment_method.class}")
まあ、これはあくまで「悪いコード例」なので、がんばってきれいにする必要はないのかもしれませんが・・・。
P31: payメソッドやtransferメソッドが定義されていない（サンプルコードとしては許容範囲か？）
こちらも先ほどの悪いコード例です。
class Customer
def pay(payment_method)
case payment_method.class.to_s
when 'CreditCard'
payment_method.pay
when 'Bank', 'Conveni', 'PostOffice'
payment_method.transfer
else
raise ArgumentError.new('unknown payment method')
end
end
end
class CreditCard end
class Bank end
class ConvenienceStore end
class PostOffice end
customer = Customer.new
customer.pay(CreditCard.new)
when節の処理として "payment_method.pay" や "payment_method.transfer" がありますが、これらのメソッドはどこにも定義されていません。
なので、このサンプルコードをそのまま実行すると以下のエラーが発生します。
irb(main):023:0> customer.pay(CreditCard.new)
NoMethodError: undefined method `pay' for #<CreditCard:0x007fc43b827cb8>
from (irb):8:in `pay'
from (irb):23
from /Users/jit/.rbenv/versions/2.4.1/bin/irb:11:in `<main>'
実際に動かす場合は次のように、メソッドの定義も必要になります。
class CreditCard
def pay; end
end
class Bank
def transfer; end
end
class ConvenienceStore
def transfer; end
end
class PostOffice
def transfer; end
end
まあ、あくまで「悪いコード例」ですし、記事でフォーカスしているのは「特定のクラスに依存する条件分岐を作るな」という点なので、こんなところにツッコむのは野暮だとは自覚していますが。。。
でも、明らかに動かないコードを見せられるとちょっとモヤモヤしてしまいます。
P33: settlementは名詞 / Settlementableもおかしい
クレジットカードクラスのサンプルコードとして登場した以下のコードにも気になる部分がありました。
class CreditCard
attr_accessor :number, :expiration_date, :balance
def settlement
end
def register
end
def charge
end
end
よく使われるメソッドとして登場している "settlement" メソッドですが、 "settlement" の品詞は名詞です。
「決済する」メソッドなのであれば、動詞の "settle" を使うか、 "make_settlement" のように「動詞＋目的語」の形にする方が望ましいです。
（クレジットカード業界ではどういう英語表現を使うのか僕はよく知りませんが。。。）
また、P33に「このケースでは、Settlementable(決済可能)というロールになるかもしれません」とありますが、接尾辞の-ableは動詞に付けくのでSettlementableではなくSettleableにした方が良いでしょう。（ただし、ネイティブの人が見たらSettleableも違和感があるかもしれません）
参考： モデルやメソッドに名前を付けるときは英語の品詞に気をつけよう - Qiita
P36: セミコロンとif文のカッコは不要
P36に登場している以下のコードですが、僕だったらセミコロンとif文のカッコはなくします。
val = 1;
if ([1, 2].include?(val))
end
val = 1;
if (1 == val || 2 == val)
end
つまり、こんな感じです。
val = 1
if [1, 2].include?(val)
end
val = 1
if 1 == val || 2 == val
end
もしかすると、開発の現場によっては「if文には必ずカッコを付けるべし」というコード規約があるのかもしれませんが、個人的な観測範囲ではそういうルールは見かけたことがありません。
あと、余談ですが、2つ目のif文は僕だったら "val == 1" のように書きますね。
この方が「valが1だったら」というふうにコードが読めるので。
val = 1
if val == 1 || val == 2
end
（2017.6.30 20:00追記）
「val == 1 ではなく 1 == val と書くのは、間違って val = 1 と書いてしまって val に値を代入してしまうバグを避けるためじゃないか」という意見が散見されました。
なるほど、そういえばそういう話を昔どこかで読んだ気がします。
ただ、 val == 1 も 1 == val も、どちらもメリットとデメリットがありますね。
- val == 1 なら読みやすいが、間違って val = 1 と書いてしまう可能性がゼロではない。
- 1 == val なら間違って val を書き換えてしまう可能性はないが、若干読みづらい。
こうなると、好みの世界のような気がします。
僕はどっちかひとつを選べと言われたら、 val == 1 を選びます！
僕は読みやすさの方が大事ですし、 val = 1 と間違って書いてしまったことも今のところありません。
「いやいや、そうは言っても・・・」と話し出すと宗教論争になってしまうので、これはみなさんが自分の開発チームでどちらがいいのか（もしくはどちらでもいいのか）を話合ってもらうのが一番よいと思います。
まとめ
というわけでこのエントリでは、WEB+DB PRESS Vol.99の「Rubyで学ぶ！良いコードって何だろう？」に出てきたサンプルコードを、個人的な基準でコードレビューしてみました。
いやあ～、我ながら実に細かいですね。
とはいえ、「良いコード」を議論するのであれば、サンプルコードも徹底的に隙の無いレベルにしておいた方がいいと僕は思いました。
「この部分は参考になるけど、ここはあまり参考にならない」というのも変なので。
もちろん、「良いコード」「悪いコード」の判断基準は個人によって、そして開発の現場によって多少異なるものです。
みなさんの中には、僕が挙げた指摘事項を読みながら「えー、それはそのままでもいいじゃん」と思った人もいるかもしれません。
良いコードとは何か、もっと真剣に考えてみたい！という方はぜひ、WEB+DB PRESS Vol.99を手にとってみてください。
記事を読みながら「うん、それはそうだ！」「なるほど、そういう考え方もあるのか」「え～、このコードは微妙かも」・・・といった議論を、開発チーム内でやってみると面白いかもしれません。
この特集記事と僕の今回のエントリをきっかけにして、みなさんの「良いコードってなんだろう？」を追求してみましょう！
WEB+DB PRESS Vol.99
- 作者: ?橋健一,谷口禎英,井本大登,山崎勝平,大和田純,内村元樹,坂東昌哉,平田敏之,牧大輔,板敷康洋,大?浩崇,穴井宏幸,原口宗悟,久田真寛,ふしはらかん,のざきひろふみ,うらがみ,ひげぽん,池田拓司,はまちや2,竹原,片田雄樹,渋江一晃,WEB+DB PRESS編集部編
- 出版社/メーカー: 技術評論社
- 発売日: 2017/06/24
- メディア: 大型本
- この商品を含むブログを見る
お知らせ：「プロを目指す人のためのRuby入門」もぜひ！
Rubyでもっと良いコードを書きたい！もっとRuby言語に精通したい！という方は、現在執筆中の「プロを目指す人のためのRuby入門」も参考になるはずです！
この本は2017年11月発売予定です。
詳しくは以下のエントリをご覧ください。
あわせて読みたい
「きれいなコード」や「良いコード」というお題を出されると、黙ってはいられないんです、僕。
じゃあなんで「きれいなコード」や「良いコード」にこだわるのかというと、こういうエピソードがあるからです。
いや、なんか偉そうなこと書いてるけど、僕もRubyを始めて間もない頃はみんなからボコボコにされてたんだった・・・。
