reply comments - patch to follow
http://codereview.appspot.com/22052/diff/1/2 File lib/Cache/Memcached.pm (right): http://codereview.appspot.com/22052/diff/1/2#newcode90 Line 90: sub last_res { Fair point - i'll change that. On 2009/02/28 15:58:41, bradfitz wrote:
"res" is a great internal abbreviations but it's a terrible symbol to
let leak
out in a public API. This should be renamed to "last_result"
probably. http://codereview.appspot.com/22052/diff/1/2#newcode414 Line 414: $self->{last_res} =~ s/\r\n$//; Also a good suggestion. On 2009/02/28 15:58:41, bradfitz wrote:
Performance suggestion: It might make more sense to only do the \r\n
stripping
lazily, when the user actually calls the "last_result" method, so you
avoid the
regexp in the common case (of the user not calling the method).
http://codereview.appspot.com/22052/diff/1/2#newcode1112 Line 1112: This method returns the status string of the last operation except for get(s). The whole module has no support or mention of the binary protocol, but I would envision binary support to work the way you mention. No point having a pile of constants that have to be exported/imported when we can use the simple strings of the ascii protocol. I'll include some sort of comment to that effect. On 2009/02/28 15:58:41, bradfitz wrote:
So this only works with the ASCII protocol? You should be explicit
about that.
Perhaps document that if this module were ever to support the binary
protocol,
this method would continue to return the ASCII *equivalent* status
code, even if
the library decided to speak binary behind the scenes.
http://codereview.appspot.com/22052
