Skip to content

Instantly share code, notes, and snippets.

@rirakkumya
Created April 19, 2012 15:14
Show Gist options
  • Save rirakkumya/2421593 to your computer and use it in GitHub Desktop.
Save rirakkumya/2421593 to your computer and use it in GitHub Desktop.
def foo[A,B](in:Option[A])(valid:PartialFunction[Option[A],Either[B,A]])(invalid:PartialFunction[Option[A],Either[B,A]])(other: =>Either[B,A]) = {
if(!valid.isDefinedAt(in) && !invalid.isDefinedAt(in)) {
other
}else{
(valid orElse invalid)(in)
}
}
def getFirstData(id:String) = {
foo(Cache.get(id)){
case Some(r) if r.startsWith("x") => Right("foo")
case Some(r) => Right(r)
}{
case Some(r) if r == "badcode" => Left(BadRequest)
}{
Left(NotFound)
}
}
@rirakkumya
Copy link
Author

暗黙の型変換でLeft,Right消せそう。あとなんか関数使えばもっといけてる感じにならないかな。

@lyricallogical
Copy link

すいません、率直に意見を述べさせて頂きます。

foo の定義を知らずにこの getFirstData を読んで、何がどう動くか分かる人は恐らくいないと思うので、分かりやすさはかなり悪いと思います。
まず最初の PartialFunction と、次の PartialFunction と、最後のブロック(単なる値を返す式)が何を意味するのか、仮に foo をどのような名前にしてやったとしても、getFirstData を読む人には伝わらないのではないでしょうか。
named argument を利用すれば、少しましになるとは思います(ついでに不要になったのでパラメタのカリー化もやめてます)

def getFirstData(id:String) = {
  foo(Cache.get(id))(valid = {
    case Some(r) if r.startsWith("x") => Right("foo")
    case Some(r) => Right(r)
  }, invalid = {
    case Some(r) if r == "badcode" => Left(BadRequest)
  }, other = {
    Left(NotFound)
  });
}

しかし、そもそも valid, invalid, other とは何なのか、という話になってきます。valid が必ず Right を、invalid が必ず Left を返すものだということなら、ParialFunction の型パラメタはどちらも Either である必要はないように思います。other も同様です。。

そして何よりも、このコードが分かりにくいことは、この getFirstData の実装では invalid が必ず実行されることがないということが、はっきりと証明しているのではないでしょうか。Cache.get(id) が Some("badcode") を返した場合、invalid に到達するまえに valid が処理してしまうため、Right("badcode") が返されてしまいます。

ボクの validateWith による実装は、「内側」で更なる validate 処理が必要になった場合に cont: String => String ではどうしようもなくなる、という点で rigid なコードだったと思います。それは書いた時点で分かっていました。が、そうはならないという前提があるつもりで書いていました。

そんなところです。success, failure, error とは何か、という点があやふやなコードになってしまっているのが、一番問題だと感じました。

@lyricallogical
Copy link

折角なのでもう少しだけ書かせてください。

基本的に、問題の発生箇所とその例外処理が離れれば離れるほど、コードは読みにくくなると思っています。
具体的には下のような話です。

def bad() =
  if (success) {
    ... longlonglonglong process
  } else {
    valueForNotSuccess
  }

def good() = {
  if (!success) {
    return valueForNotSuccess
  }
  ... longlonglonglong process
}

return は使うべきでないとする主張もあるとは思いますが、そこはまあ置いておいて…
これはサンプルコードなので、まあどちらも大差なくみえますが、longlonglonglong processs が非常に縦に長かったとしたらどうでしょうか。もちろん、そもそもそんな長い処理は関数として抽出しろ、というのは全く正しい主張だと思うのですが…そこは本質ではないと思います。

というような視点で見ても、矢張り rirakkumya さんのコードは読みにくいということがいえるのではないかと思います。None だった場合には NotFound を返す、という例外処理が、Cache.get から離れてしまっているためです。

(余談ですが、ボクは try catch による例外処理は、「距離」を長くしがちなので「構文として」好きではないです。そもそも「例外」自体好きではないですが…)

長々とすいませんでした。以上です。
コードの readability に関しては、様々な視点があり、多くの場合、例外処理の最適解はケースバイケースなのだと思います。ボクが上述した主張が常に正しいとも思っていません。というわけで、こういう視点もあるよ、という意見は大歓迎です。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment