Created
April 19, 2012 15:14
-
-
Save rirakkumya/2421593 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | |
} | |
} |
折角なのでもう少しだけ書かせてください。
基本的に、問題の発生箇所とその例外処理が離れれば離れるほど、コードは読みにくくなると思っています。
具体的には下のような話です。
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
すいません、率直に意見を述べさせて頂きます。
foo の定義を知らずにこの getFirstData を読んで、何がどう動くか分かる人は恐らくいないと思うので、分かりやすさはかなり悪いと思います。
まず最初の PartialFunction と、次の PartialFunction と、最後のブロック(単なる値を返す式)が何を意味するのか、仮に foo をどのような名前にしてやったとしても、getFirstData を読む人には伝わらないのではないでしょうか。
named argument を利用すれば、少しましになるとは思います(ついでに不要になったのでパラメタのカリー化もやめてます)
しかし、そもそも 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 とは何か、という点があやふやなコードになってしまっているのが、一番問題だと感じました。