Vol.99の「良いコード」を本気でコードレビューしてみたRubyはじめにTwitterを見てたら、気になる雑誌の特集を見つけました。Vol.99の「Rubyで学ぶ!良いコードって何だろう?」という特集記事です。大型本この商品を含むブログを見るRuby大好き!きれいなコード大好き!!な僕にとっては、この特集は読まずにはいられません!早速買って読んでみました。お~、なるほど、たしかにいいことが書いてある!うんうん、そうそう・・・あれ?このコード、どうなん?うーん、この例はちょっと微妙かも。思ったところがちーらほら。重箱の隅を突っつくような話をするのは野暮だなあとは思いつつ、それでも「良いコード」をうたう特集記事なのであれば、隅から隅まで「良いコード」を貫き通してほしい!というわけで、このエントリではこの特集記事を読んでいて、ちょっと気になったサンプルコードをピックアップしていこうと思います。中には「そんな細かいところまでチェックしなくても」と思われるものもあるかもしれませんが、せっかくのいい特集なので、サンプルコードは徹底的にブラッシュアップした方がいいんじゃないかと思い、このエントリを書きました。Vol.99を読んでいないみなさんへ僕のエントリだけ読むと、「この記事、問題だらけなん?」って思われるかもしれませんが、そうではありません!記事自体は非常に良い内容になっていますし、僕もほとんどの内容に同意しています。説明の文章が非常に読みやすいのも、この記事のいいところだと思います。Vol.99を購入してもらっても損はありません。安心して購入してください!とはいえ、真剣に僕基準でサンプルコードをレビューしていくと、それなりの件数の改善ポイントが見つかりました。僕はソニックガーデンの社内でも「一番コードにうるさくて、面倒くさい男」と思われてるそうなので(苦笑)、僕が仕事でコードレビューするなら、たぶんこれぐらいツッコミを入れます。PRESS編集部のみなさんへこのエントリが引用の範囲を超えていると判断された場合はエントリを削除しますので、ご連絡ください。PRESS内で使われているサンプルコードはGitHubでも公開されています。それでは以下が本編です。何もしないelseは必要か?記事の一番最初に出てきたこのコード例ですが、いきなり「すごく微妙なライン」を持ってきたな~という印象です。以外の場合に何もしないのは意図したことなのか不安が残るのに対し、下のコードでは何もしなくてよいという書き手の意図が読み手にもしっかりと伝わります。」とのことですが、この「何もしないelse」が必須かと言われると、うーん。僕だったら「何もしないelse」は作らないですね。このコード例であれば「elseがない=9時と18時以外は何もしない」と解釈します。「elseがないとコードを読んだ人が明らかに不安に感じる」という特殊なケースでは「何もしないelse」を書くことも検討しますが、このコード例のレベルであれば僕はelseは書きません。13:30追記)あえて何もしない条件分岐を作る場合、こういうコードならまだアリかな~と思います。24ここでは「予期しない時間(hour)」が渡ってきたときに、問題にすぐ気づけるようエラーを発生させています。条件(hourが0~23なら真になる条件)を入れています。when節の中身は空っぽです。以外の場合に何もしない」という意図が、よりハッキリするんじゃないかと思います。12:00追記)あと、「何もしないelse」を書く代わりにテストコードを書いておくのも有効だと思います。すれば「開発者はこの条件だけを処理の対象にしたいと考えている」という意図がよりわかると思うので。設計の意図が全く読めないサンプルコード続いてはこちらのコード例です。nil:NilClassこのコードは変数のスコープが広すぎる悪い例として挙げられているものです。何度も代入したいものなのでしょうか。初期化時だけでは駄目なのでしょうか。何度も代入したいものなのでしょうか。初期化時だけでは駄目なのでしょうか。」という問いに対する答えは、「要件による」としか言いようがありません。このサンプルコードでは、Fooクラスと変数barの役割が全くわかりません。method_1、method_2、method_3も何をするメソッドか全くわかりません。いい名前が付いているメソッドであれば「barがnilに変わるケースもありそう」と思えるかもしれません。言わんとしていることはわかります。「不用意にオブジェクトの状態を変更できるようにすべきではない」「無用な副作用をメソッドに持たせてはいけない」というのは、良いコードの大事な条件です。ここでは何をやろうとしているのかわからないコード例になっているため、いまいち説得力に欠けているのが残念なポイントです。ちなみに、まったく同じテーマではないですが、少し似た話をその昔Qiitaに書いたことがあります。initメソッド?initializeメソッド?次はマジックナンバーではなく、定数を使おうというこのサンプルコードです。endend定数を使うのはOKなのですが、initメソッドはいったい何なのでしょう?initializeメソッド(JavaやC#でいうところのコンストラクタ)とは別のメソッドなのでしょうか?このコードを見る限り、initializeメソッドでよいと思います。しないと、newしたあとに、わざわざinitメソッドを呼び出さなければならないからです。initメソッドなのか、initializeメソッドと書くつもりだったのか、意図がつかめずモヤモヤします。定数はドットでは参照できない(エラーが起きる)さて、次はこちらのコードです。"クレジットカードの情報が誤っています。(ドットではなくダブルコロンで区切る)が正ですね。メソッドになっていなければなりません。定数なのでエラーが起きます。`<main>'ダブルコロンなら問題ありません。100うっかりミスだとは思いますが、このへんはきちっとコードレビューしてほしかったな~と思うところです。例外クラスに定数を定義すべき?(状況による)先ほどと同じこちらのコードです。101endもし、これが定数しか持たないクラスなのであれば、クラスではなくモジュールとして定義する方が良いでしょう。ExceptionCodeはインスタンス化する必要がないからです。101endただし、これはあくまで短いサンプルコードですが、実務レベルのコードではインスタンス化する必要が出てくるかもしれません。クラスにすべきか、モジュールにすべきかは、そのときの状況によります。ようにしてエラーコードを比較するのであれば、ServiceExceptionクラスでこれらの定数を定義している方が設計として自然だと思います。"クレジットカードの情報が誤っています。Rubyの例外クラス名は慣習的に~Errorで終わることが多いまたまた先ほどのコードを登場させます。"クレジットカードの情報が誤っています。節にServiceExceptionという例外クラスが登場していますが、Rubyの実行時例外は通常、StandardErrorクラスを継承します。StandardErrorクラスを継承した場合は、例外クラスの名前も~Errorで終わることが多いです。方がRubyっぽい例外クラス名だと言えます。これは慣習的な話なので、必ずそうすべきというルールではありません。メソッドとは?もう一度、先ほどのコードに登場してもらいます(4回目)。"クレジットカードの情報が誤っています。いうビックリマーク付きの制御構造やメソッドは標準では存在しません。どこかで定義されている独自のメソッドなのかもしれませんが、例外処理の中でそのようなメソッドを使うとRuby標準のretryと混同しやすいので、あまり良いメソッド名だとは言えません。予想ですが、これは本来retryと書くつもりだったんじゃないかと思います。redirectメソッド??またまた先ほどのコードを出します。"クレジットカードの情報が誤っています。これがもしRailsのコードだとしたら、redirectではなく、redirect_toが正です。記事の中にはどこにも「Railsのコードである」との言明はないため、架空のフレームワークに定義された架空のメソッドである可能性もあります。いえ、user_creditcard_pathというメソッドもRailsっぽいです。これがRailsっぽくてRailsでないコードなのであれば、読者を困惑させる原因になります。「これはRailsのコードです」もしくは「これはあくまでRailsではない架空のWebアプリケーションです」との言明が本文中にほしかったなと思います。使われないローカル変数を登場させない以下のコードの上から3行目、create_orderの戻り値をorderという変数に格納していますが、このorderは使われないローカル変数です。良いコード例として紹介するのであれば、使われないローカル変数は登場させない方がいいと思います。メソッドの名前は「動詞+目的語」の方が自然もう一度先ほどのコードを登場させます。ように「動詞+目的語」の語順にした方がメソッド名として自然です。インデントが1文字深い下記コードの3行目、インデントが1文字ぶん深いです。クラスは文字列にしなくてもwhen節に直接クラスを指定できる以下はクラス名に依存する悪いコード例として出てきたものなので、ツッコむのも野暮かもしれませんが、気になったので一応書いておきます。endendRubyではwhen節に直接クラスを指定できるので、上のコードは次のように書いた方がシンプルです。endendただし、「もしかすると」ですが、この言語仕様はRubyの中でも比較的マイナーな部類に入る(と思う)ので、Rubyに詳しくない人でもわかるようにあえて文字列で比較したのかな?という気がしなくもありません。Conveniクラスがない先ほど出てきたコードをもう一度登場させます。クラス名が異なるので、意図した通りに動きません。このコードはあくまで「悪いコード例」ですが、僕がここで指摘した内容は本文中に「悪い部分」として取り上げられていないので、おそらく純粋なコーディングミスだと予想します。例外を発生させるならデバッグしやすい情報を含めるべきまたまた先ほどの「悪いコード例」です。endend想定外の選択肢(つまりelse節)に入ってきたら例外を発生させる、という考え方は良いのですが、どんなオブジェクトがやってきたのかわからないと、例外が起きた後のデバッグに苦労します。例外メッセージには次のようにpayment_methodのクラス名も含めた方が良いでしょう。#{payment_method.class}")まあ、これはあくまで「悪いコード例」なので、がんばってきれいにする必要はないのかもしれませんが・・・。payメソッドやtransferメソッドが定義されていない(サンプルコードとしては許容範囲か?)こちらも先ほどの悪いコード例です。ありますが、これらのメソッドはどこにも定義されていません。このサンプルコードをそのまま実行すると以下のエラーが発生します。`<main>'実際に動かす場合は次のように、メソッドの定義も必要になります。endendまあ、あくまで「悪いコード例」ですし、記事でフォーカスしているのは「特定のクラスに依存する条件分岐を作るな」という点なので、こんなところにツッコむのは野暮だとは自覚していますが。明らかに動かないコードを見せられるとちょっとモヤモヤしてしまいます。Settlementableもおかしいクレジットカードクラスのサンプルコードとして登場した以下のコードにも気になる部分がありました。ように「動詞+目的語」の形にする方が望ましいです。(クレジットカード業界ではどういう英語表現を使うのか僕はよく知りませんが。)また、P33に「このケースでは、Settlementable(決済可能)というロールになるかもしれません」とありますが、接尾辞の-ableは動詞に付けくのでSettlementableではなくSettleableにした方が良いでしょう。セミコロンとif文のカッコは不要P36に登場している以下のコードですが、僕だったらセミコロンとif文のカッコはなくします。処理endつまり、こんな感じです。処理endもしかすると、開発の現場によっては「if文には必ずカッコを付けるべし」というコード規約があるのかもしれませんが、個人的な観測範囲ではそういうルールは見かけたことがありません。この方が「valが1だったら」というふうにコードが読めるので。値を代入してしまうバグを避けるためじゃないか」という意見が散見されました。いえばそういう話を昔どこかで読んだ気がします。どちらもメリットとデメリットがありますね。書いてしまう可能性がゼロではない。書き換えてしまう可能性はないが、若干読みづらい。なると、好みの世界のような気がします。間違って書いてしまったことも今のところありません。「いやいや、そうは言っても・・・」と話し出すと宗教論争になってしまうので、これはみなさんが自分の開発チームでどちらがいいのか(もしくはどちらでもいいのか)を話合ってもらうのが一番よいと思います。Vol.99の「Rubyで学ぶ!良いコードって何だろう?」に出てきたサンプルコードを、個人的な基準でコードレビューしてみました。いやあ~、我ながら実に細かいですね。いえ、「良いコード」を議論するのであれば、サンプルコードも徹底的に隙の無いレベルにしておいた方がいいと僕は思いました。「この部分は参考になるけど、ここはあまり参考にならない」というのも変なので。「良いコード」「悪いコード」の判断基準は個人によって、そして開発の現場によって多少異なるものです。みなさんの中には、僕が挙げた指摘事項を読みながら「えー、それはそのままでもいいじゃん」と思った人もいるかもしれません。Vol.99を手にとってみてください。記事を読みながら「うん、それはそうだ!」「なるほど、そういう考え方もあるのか」「え~、このコードは微妙かも」・・・といった議論を、開発チーム内でやってみると面白いかもしれません。大型本この商品を含むブログを見るお知らせ:「プロを目指す人のためのRuby入門」もぜひ!Rubyでもっと良いコードを書きたい!もっとRuby言語に精通したい!という方は、現在執筆中の「プロを目指す人のためのRuby入門」も参考になるはずです!この本は2017年11月発売予定です。詳しくは以下のエントリをご覧ください。あわせて読みたい「きれいなコード」や「良いコード」というお題を出されると、黙ってはいられないんです、僕。なんで「きれいなコード」や「良いコード」にこだわるのかというと、こういうエピソードがあるからです。いや、なんか偉そうなこと書いてるけど、僕もRubyを始めて間もない頃はみんなからボコボコにされてたんだった・・・。先月、CodeIQにビンゴカード作成問題を出題しました。はてなブログPro株式会社ソニックガーデンで働くRubyプログラマ。2017年11月に「プロを目指す人のためのRuby入門」を出版。blog)はてなブログをはじめよう!JunichiItoさんは、はてなブログを使っています。ブログを報告するスターをつけました引用をストックしましたストック一覧を見る閉じる引用するにはまずログインしてくださいログイン閉じる引用をストックできませんでした。再度お試しください閉じる限定公開記事のため引用できません。