はじめに
Twitterを見てたら、気になる雑誌の特集を見つけました。
WEB+DB PRESS Vol.99の「Rubyで学ぶ!良いコードって何だろう?」という特集記事です。
- 作者: ?橋健一,谷口禎英,井本大登,山崎勝平,大和田純,内村元樹,坂東昌哉,平田敏之,牧大輔,板敷康洋,大?浩崇,穴井宏幸,原口宗悟,久田真寛,ふしはらかん,のざきひろふみ,うらがみ,ひげぽん,池田拓司,はまちや2,竹原,片田雄樹,渋江一晃,WEB+DB PRESS編集部編
- 出版社/メーカー: 技術評論社
- 発売日: 2017/06/24
- メディア: 大型本
- この商品を含むブログを見る
早速買って読んでみました。
お~、なるほど、たしかにいいことが書いてある!
うんうん、そうそう・・・あれ?このコード、どうなん?
うーん、この例はちょっと微妙かも。。
と思ったところがちーらほら。
重箱の隅を突っつくような話をするのは野暮だなあとは思いつつ、それでも「良いコード」をうたう特集記事なのであれば、隅から隅まで「良いコード」を貫き通してほしい!
というわけで、このエントリではこの特集記事を読んでいて、ちょっと気になったサンプルコードをピックアップしていこうと思います。
本編へ進む前に
執筆者のみなさまへ
せっかくがんばって書いた記事に、あれこれいちゃもん付けてすいません!
先に謝っておきます。🙇🙇🙇
指摘事項の中には「そんな細かいところまでチェックしなくても」と思われるものもあるかもしれませんが、せっかくのいい特集なので、サンプルコードは徹底的にブラッシュアップした方がいいんじゃないかと思い、このエントリを書きました。
まだ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) #=> nil greeting(9) #=> "Hello" greeting(18) #=> "Good-bye" greeting(23) #=> nil greeting(24) #=> ArgumentError: Invalid hour: 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") # => NoMethodError: undefined method `+' for nil:NilClass
このコードは変数のスコープが広すぎる悪い例として挙げられているものです。
記事によると、「どうやらどこかで bar に nil が入ってしまっ たようです。(中略) そもそもこの bar は何度も代入したいものなのでしょうか。初期化時だけでは駄目なのでしょうか。」とのことなんですが、「そもそもこの bar は何度も代入したいものなのでしょうか。初期化時だけでは駄目なのでしょうか。」という問いに対する答えは、「要件による」としか言いようがありません。
しかし、このサンプルコードでは、Fooクラスと変数barの役割が全くわかりません。
また、method_1、method_2、method_3も何をするメソッドか全くわかりません。
いい名前が付いているメソッドであれば「barがnilに変わるケースもありそう」と思えるかもしれません。
もちろん、言わんとしていることはわかります。
「不用意にオブジェクトの状態を変更できるようにすべきではない」「無用な副作用をメソッドに持たせてはいけない」というのは、良いコードの大事な条件です。
ですが、ここでは何をやろうとしているのかわからないコード例になっているため、いまいち説得力に欠けているのが残念なポイントです。
ちなみに、まったく同じテーマではないですが、少し似た話をその昔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" のように「動詞+目的語」の語順にした方がメソッド名として自然です。
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も違和感があるかもしれません)
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を手にとってみてください。
記事を読みながら「うん、それはそうだ!」「なるほど、そういう考え方もあるのか」「え~、このコードは微妙かも」・・・といった議論を、開発チーム内でやってみると面白いかもしれません。
この特集記事と僕の今回のエントリをきっかけにして、みなさんの「良いコードってなんだろう?」を追求してみましょう!
- 作者: ?橋健一,谷口禎英,井本大登,山崎勝平,大和田純,内村元樹,坂東昌哉,平田敏之,牧大輔,板敷康洋,大?浩崇,穴井宏幸,原口宗悟,久田真寛,ふしはらかん,のざきひろふみ,うらがみ,ひげぽん,池田拓司,はまちや2,竹原,片田雄樹,渋江一晃,WEB+DB PRESS編集部編
- 出版社/メーカー: 技術評論社
- 発売日: 2017/06/24
- メディア: 大型本
- この商品を含むブログを見る
お知らせ:「プロを目指す人のためのRuby入門」もぜひ!
Rubyでもっと良いコードを書きたい!もっとRuby言語に精通したい!という方は、現在執筆中の「プロを目指す人のためのRuby入門」も参考になるはずです!
この本は2017年11月発売予定です。
詳しくは以下のエントリをご覧ください。
あわせて読みたい
「きれいなコード」や「良いコード」というお題を出されると、黙ってはいられないんです、僕。
じゃあなんで「きれいなコード」や「良いコード」にこだわるのかというと、こういうエピソードがあるからです。
いや、なんか偉そうなこと書いてるけど、僕もRubyを始めて間もない頃はみんなからボコボコにされてたんだった・・・。