-
-
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) | |
} | |
} |
すいません、率直に意見を述べさせて頂きます。
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 とは何か、という点があやふやなコードになってしまっているのが、一番問題だと感じました。
折角なのでもう少しだけ書かせてください。
基本的に、問題の発生箇所とその例外処理が離れれば離れるほど、コードは読みにくくなると思っています。
具体的には下のような話です。
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 に関しては、様々な視点があり、多くの場合、例外処理の最適解はケースバイケースなのだと思います。ボクが上述した主張が常に正しいとも思っていません。というわけで、こういう視点もあるよ、という意見は大歓迎です。
暗黙の型変換でLeft,Right消せそう。あとなんか関数使えばもっといけてる感じにならないかな。