Refactor MakeRequestHandler and MakeRequestServlet into a utility class

[chabotc, 2009-08-13 19:25:11]

Code looks really great, that's a really a very nice cleanup!

http://codereview.appspot.com/105099/diff/9/1022
File src/gadgets/MakeRequest.php (right):

http://codereview.appspot.com/105099/diff/9/1022#newcode21
Line 21: /**
This being a somewhat complex and very useful class that is used in a few
different locations, it might be useful to include a usage code example in the
class code doc?

http://codereview.appspot.com/105099/diff/9/1022#newcode72
Line 72: throw new Exception($name . " parameter must be a boolean value");
When a class has many internal exception possibilities, I usually prefer
defining it's own exception type or use the type defined by it's calling class.

In this case having a MakeRequestException seems appropiate

http://codereview.appspot.com/105099/diff/9/1021
File src/gadgets/MakeRequestHandler.php (right):

http://codereview.appspot.com/105099/diff/9/1021#newcode54
Line 54: private function setValueFromRequest(&$dest, $name, $default = null,
$type = null) {
Parsing variables by reference isn't very 'PHPish' and usually is best avoided
if equal alternatives exist.

Sometimes passing large structures with Kb's of data by reference can be a
useful optimization, but in this case they seem to be regular vars.

As such I would just return the resulting variable and allow the assigning to be
done by the calling logic

http://codereview.appspot.com/105099/diff/9/1021#newcode85
Line 85: $this->setValueFromRequest($params['format'], 'contentType');
These would be easier to understand (for a PHP'r) if they were assigned like:
$params['format'] = $this->setValueFromRequest('contentType');

[Arne Roomann-Kurrik, 2009-08-15 01:36:39]

I realized that configuring the makeRequest class using an array was really more
JavaScript than PHP, and was probably prone to breaking once people started
using it in multiple places.  I especially didn't like that most of the
parameters had arbitrary formatting, either in caps, camel case, or using
underscores instead of spaces.

So I refactored the parts of the code that built up the parameter array into a
MakeRequestOptions class.  It adds a little bit of code, but at least now it's
harder to use the wrong parameter when configuring makeRequest.

This refactoring took care of most of the feedback you left.  I also made sure
to put in a couple examples on how to configure the class and send a request.  

~Arne

On 2009/08/13 19:25:11, chabotc wrote:
> Code looks really great, that's a really a very nice cleanup!
> 
> http://codereview.appspot.com/105099/diff/9/1022
> File src/gadgets/MakeRequest.php (right):
> 
> http://codereview.appspot.com/105099/diff/9/1022#newcode21
> Line 21: /**
> This being a somewhat complex and very useful class that is used in a few
> different locations, it might be useful to include a usage code example in the
> class code doc?
> 
> http://codereview.appspot.com/105099/diff/9/1022#newcode72
> Line 72: throw new Exception($name . " parameter must be a boolean value");
> When a class has many internal exception possibilities, I usually prefer
> defining it's own exception type or use the type defined by it's calling
class.
> 
> In this case having a MakeRequestException seems appropiate
> 
> http://codereview.appspot.com/105099/diff/9/1021
> File src/gadgets/MakeRequestHandler.php (right):
> 
> http://codereview.appspot.com/105099/diff/9/1021#newcode54
> Line 54: private function setValueFromRequest(&$dest, $name, $default = null,
> $type = null) {
> Parsing variables by reference isn't very 'PHPish' and usually is best avoided
> if equal alternatives exist.
> 
> Sometimes passing large structures with Kb's of data by reference can be a
> useful optimization, but in this case they seem to be regular vars.
> 
> As such I would just return the resulting variable and allow the assigning to
be
> done by the calling logic
> 
> http://codereview.appspot.com/105099/diff/9/1021#newcode85
> Line 85: $this->setValueFromRequest($params['format'], 'contentType');
> These would be easier to understand (for a PHP'r) if they were assigned like:
> $params['format'] = $this->setValueFromRequest('contentType');

[Arne Roomann-Kurrik, 2009-08-24 17:24:28]

Ping on this review.

~Arne


On 2009/08/15 01:36:39, Arne Roomann-Kurrik wrote:
> I realized that configuring the makeRequest class using an array was really
more
> JavaScript than PHP, and was probably prone to breaking once people started
> using it in multiple places.  I especially didn't like that most of the
> parameters had arbitrary formatting, either in caps, camel case, or using
> underscores instead of spaces.
> 
> So I refactored the parts of the code that built up the parameter array into a
> MakeRequestOptions class.  It adds a little bit of code, but at least now it's
> harder to use the wrong parameter when configuring makeRequest.
> 
> This refactoring took care of most of the feedback you left.  I also made sure
> to put in a couple examples on how to configure the class and send a request. 

> 
> ~Arne
> 
> On 2009/08/13 19:25:11, chabotc wrote:
> > Code looks really great, that's a really a very nice cleanup!
> > 
> > http://codereview.appspot.com/105099/diff/9/1022
> > File src/gadgets/MakeRequest.php (right):
> > 
> > http://codereview.appspot.com/105099/diff/9/1022#newcode21
> > Line 21: /**
> > This being a somewhat complex and very useful class that is used in a few
> > different locations, it might be useful to include a usage code example in
the
> > class code doc?
> > 
> > http://codereview.appspot.com/105099/diff/9/1022#newcode72
> > Line 72: throw new Exception($name . " parameter must be a boolean value");
> > When a class has many internal exception possibilities, I usually prefer
> > defining it's own exception type or use the type defined by it's calling
> class.
> > 
> > In this case having a MakeRequestException seems appropiate
> > 
> > http://codereview.appspot.com/105099/diff/9/1021
> > File src/gadgets/MakeRequestHandler.php (right):
> > 
> > http://codereview.appspot.com/105099/diff/9/1021#newcode54
> > Line 54: private function setValueFromRequest(&$dest, $name, $default =
null,
> > $type = null) {
> > Parsing variables by reference isn't very 'PHPish' and usually is best
avoided
> > if equal alternatives exist.
> > 
> > Sometimes passing large structures with Kb's of data by reference can be a
> > useful optimization, but in this case they seem to be regular vars.
> > 
> > As such I would just return the resulting variable and allow the assigning
to
> be
> > done by the calling logic
> > 
> > http://codereview.appspot.com/105099/diff/9/1021#newcode85
> > Line 85: $this->setValueFromRequest($params['format'], 'contentType');
> > These would be easier to understand (for a PHP'r) if they were assigned
like:
> > $params['format'] = $this->setValueFromRequest('contentType');

[chabotc, 2009-09-05 14:21:38]

Patch LGTM, the only small hesitation i had is if the MakeRequestOptions class
should be included in the MakeRequest file (to many files tends to slow PHP down
by quite a bit), but it didn't seem a big enough deal to change that right now.

Sorry it took a while for it to be committed! Please include chabotc@google.com
in the reviewers list in the future since i hardly -ever- check my gmail mail
and it risks me missing such important things ;/

[Arne Roomann-Kurrik, 2009-09-08 17:44:48]

Thanks!  I'll try and put the MakeRequestOptions class into MakeRequest.php in a
future patch.

Now for osapi.http support :)

~Arne

On 2009/09/05 14:21:38, chabotc wrote:
> Patch LGTM, the only small hesitation i had is if the MakeRequestOptions class
> should be included in the MakeRequest file (to many files tends to slow PHP
down
> by quite a bit), but it didn't seem a big enough deal to change that right
now.
> 
> Sorry it took a while for it to be committed! Please include
mailto:chabotc@google.com
> in the reviewers list in the future since i hardly -ever- check my gmail mail
> and it risks me missing such important things ;/