この処理どう書いたらいいのか 迷ったねんけど...

f:id:gashoo:20171201153719j:plain


はじめに

Gashoo inc. Engineer の しゅん(@shunn_93) | Twitterです~


GASHOO Inc. Advent Calendar 2017年12月20日 を飾ります。

前回引き続き 自身の1年の振り返りとともに

コード的に どうあるべきか... スタイルガイド的に... となったときの お話です。

コーディング規約が しっかり ない時期だと よく起こる話だと 思うので, それらの対処法 を お伝えできればなと思います。



2017年10月 ~ 12月


10月   基礎的なことでは 詰まらなくなる
       あるシステムを作って欲しいという内容の設計などを考える
       周りの人からweb教えて などを言われるようになる
       エンジニアリングにおいて, できるの定義が 少しだけ明確化する

基礎部分のところで ほとんど詰まらなくなってきて,
どうあるべきか などの会話が 少しずつ できるようになる。
これが, また難しいですね。。。

また, 周りの知人から webに関する質問などが 僕の元へくるようになる。


11月   Gashoo の デザインリニューアル
    新しい機能のリリース
       react,  hanami など に手を出そうとする
       docker とはなんぞや となる
       DDDの教本を開いては 眠くなる

デザインリニューアルと 共に 新しい機能のリリースを行いました
やること多すぎて, 自分がなにやっているのか わからなくなるレベル笑

リリースするタイミングも, 障害対応したりと, なかなか 気が休まらなかった。

その後すぐに, 次どう進めていくのか, どこを修正かけていくのか, などを話し合い、
他の技術を導入検討したので, それらの勉強を始めようとした。


12月   今ココ
       体重は変わっていないが去年より 太った気がする






どう書くのがベストプラクティスなのか迷ったとき



パターン 1

def hoge
  if Time.find_by(params[:xxxx]).present?
    @drink = Drink.create(params[:xoxo])
  end
end



パターン 2

def foo
  @drink = Drink.create(params[:xoxo]) if time_present
end

private

  def time_present
    Time.find_by(params[:xxxx]).present?
  end



メソッド hoge と foo どちらも 同じ処理をしているのだが
どちらの方が 可読性があり、わかりやすいのか.....
そういった話になったことがある。

その際に どちらの パターンを採用する方がいいのか。



Ruby on Rails Style Guide には

1行は 80文字以下に統一を推奨としている。

パターン 2の長い1行の部分でも 50文字となっているので、それは 満たしている...orz

どちらの方が 読みやすいのだろうと 考えた。。。

パターン 1 は if で条件を書いて インデントしている。
パターン 2 は インデントせずに 後置if で ワンライナー(1行)で 書いている。



ここで 例に上がったのは,

私生活での会話...w

パターン 1 は

もし, 暇なんやったら, 飲み物を作って欲しい。

と 言っているイメージで,

パターン 2 は

飲み物を作って欲しい。もし, 暇なんやったら

と 言っているイメージ。

言い方であったり, 言いやすい方を 言うと思うのですが、 どちらも 同じ意味です...

パターン 1 は 相手が 暇だという前提で 言っている感じで, もう行ってきて~ と 言っている感じになりそうで...


パターン 2 は 倒置法を用いて, ほんまに 暇なんやったら、行ってきて欲しい, と 言っているニュアンスになりそうだなと...w



結論 どの方法がいいのか

考えた結果,

基本的に コードは 上から 順に読むので, パターン 1 を採用し,
尚且つ , より処理の可読性を上げるために, private method を作成し
method名から 条件を 連想できるように しました。



採用したパターン

def bar
  if hima_time_exists?
    @drink = Drink.create(params[:xoxo])
  end
end

private

  def hima_time_exists?
    Time.find_by(params[:xxxx]).present?
  end

こういう書き方にすることにより、

まず はじめに if文が 目に入り, どういう条件なのか確認する。
hima_time_exists(存在する)? と書いているので、
メソッド名だけで どういう条件なのか わかることができる。

暇な時間は存在する??という条件だ ということが メソッド名から 連想することができたら

もし, 暇なら 飲み物を 作る(買ってくると認識でも可 笑) ってことが,
barメソッドだけで確認することが 可能になる。

これがもし パターン 1 の方法を 採用していたら、
Timeってなんだっけ... params[:xxxx]って どういう情報が入っていて...
結局, どういう状態だったら、Drink.createするのだったけ....orz となる可能性が 高い....

僕は これが 一番読みやすく, わかりやすいと 思ったので, 上記を採用しました。
こういう方法の方がいいよ など ベストプラクティスな 意見がありましたら、コメントなどで 連絡してもらえると🙇


まとめ

  • メソッド名の命名など 他の人の意見を聞いてみる
    • しっかりとしたコーディング規約が あればそれに従う
    • 自分がそうだと思っていても, 連想させるモノが 違うときがあるので
  • 未来の自分を想像する
  • 可読性を上げて 読んで シンプルに なにをしているか わかるようにする

追記

  • 僕自身 リーダブルコードなどを 再度 じっくり読みます🙇

余談

前回の古川のアドベントカレンダー
仕様, 設計が 甘くて 禿げた - GASHOO BLOG



過去のGASHOO Inc. アドベントカレンダーの記事はこちら
adventar.org