Refactor post gadget rendering logic to protected methods to allow easy customization

Move the post rendering logic to protected methods for each RenderingResults.Status enum value to allow developers to extend from GadgetRenderingServlet but adding his/her own custom logic to process the rendering results.

This will help reduce the amount of code developers need to code to just have additional logic after rendering the gadget.

[henry.saputra, 2010-07-22 23:19:11]

I am thinking of adding a protected method to move the logic after rendering
done in GadgetRenderingServlet.

Hopefully this will result in less work/code duplication for developers when
customizing the GadgetRenderingServlet behavior. Developers can just extend from
GadgetRenderingServlet and override the
GadgetRenderingServlet.postGadgetRendering method to override the default
behavior.

Thoughts?

- Henry

[johnfargo, 2010-07-22 23:28:45]

http://codereview.appspot.com/1665054/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java
(right):

http://codereview.appspot.com/1665054/diff/1/2#newcode125
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java:125:
UriStatus urlstatus, GadgetContext context, RenderingResults results) throws
IOException {
I'm not opposed, but do cast a somewhat suspicious eye on a catch-all
postprocessing method that has a kitchen sink of arguments passed to it :) I'm
wondering if we can make the types of customization more precise, to encourage
reuse of common code.

Do you have any examples (even if abstract) of the types of processing desired?

What's been moved into this method is fairly generic stuff: cache handling,
error handling, redirect handling. Do you want to customize each such
subroutine? Add arbitrary content manipulation @ the end of this whole block?

[henry.saputra, 2010-07-22 23:39:34]

I agree its a kind of kitchen sink approach to overridable method.

One example I would be to have custom logic to show error message or change
encoding/content-type.

What if l refactor the code to have more specific overriable methods for each
possible result status? Maybe then it could add more flexibility to developers
with the generic logic keep inside the GadgetRenderingServlet.

- Henry

On 2010/07/22 23:28:45, johnfargo wrote:
> http://codereview.appspot.com/1665054/diff/1/2
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java
> (right):
> 
> http://codereview.appspot.com/1665054/diff/1/2#newcode125
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java:125:
> UriStatus urlstatus, GadgetContext context, RenderingResults results) throws
> IOException {
> I'm not opposed, but do cast a somewhat suspicious eye on a catch-all
> postprocessing method that has a kitchen sink of arguments passed to it :) I'm
> wondering if we can make the types of customization more precise, to encourage
> reuse of common code.
> 
> Do you have any examples (even if abstract) of the types of processing
desired?
> 
> What's been moved into this method is fairly generic stuff: cache handling,
> error handling, redirect handling. Do you want to customize each such
> subroutine? Add arbitrary content manipulation @ the end of this whole block?

[fargo, 2010-07-22 23:42:16]

That sounds good to me. You can keep this method (discouraging it being
overridden unless absolutely necessary), and then perhaps one overridable
method per RenderingResult enum?

--j

On Thu, Jul 22, 2010 at 4:39 PM, <henry.saputra@gmail.com> wrote:

> I agree its a kind of kitchen sink approach to overridable method.
>
> One example I would be to have custom logic to show error message or
> change encoding/content-type.
>
> What if l refactor the code to have more specific overriable methods for
> each possible result status? Maybe then it could add more flexibility to
> developers with the generic logic keep inside the
> GadgetRenderingServlet.
>
> - Henry
>
>
> On 2010/07/22 23:28:45, johnfargo wrote:
>
>> http://codereview.appspot.com/1665054/diff/1/2
>> File
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java
>
>> (right):
>>
>
>  http://codereview.appspot.com/1665054/diff/1/2#newcode125
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java:125:
>
>> UriStatus urlstatus, GadgetContext context, RenderingResults results)
>>
> throws
>
>> IOException {
>> I'm not opposed, but do cast a somewhat suspicious eye on a catch-all
>> postprocessing method that has a kitchen sink of arguments passed to
>>
> it :) I'm
>
>> wondering if we can make the types of customization more precise, to
>>
> encourage
>
>> reuse of common code.
>>
>
>  Do you have any examples (even if abstract) of the types of processing
>>
> desired?
>
>  What's been moved into this method is fairly generic stuff: cache
>>
> handling,
>
>> error handling, redirect handling. Do you want to customize each such
>> subroutine? Add arbitrary content manipulation @ the end of this whole
>>
> block?
>
>
>
> http://codereview.appspot.com/1665054/show
>

[henry.saputra, 2010-07-22 23:45:42]

Sounds like a plan, will post the revised version.

Thx,

- Henry

On 2010/07/22 23:42:16, fargo wrote:
> That sounds good to me. You can keep this method (discouraging it being
> overridden unless absolutely necessary), and then perhaps one overridable
> method per RenderingResult enum?
> 
> --j
> 
> On Thu, Jul 22, 2010 at 4:39 PM, <mailto:henry.saputra@gmail.com> wrote:
> 
> > I agree its a kind of kitchen sink approach to overridable method.
> >
> > One example I would be to have custom logic to show error message or
> > change encoding/content-type.
> >
> > What if l refactor the code to have more specific overriable methods for
> > each possible result status? Maybe then it could add more flexibility to
> > developers with the generic logic keep inside the
> > GadgetRenderingServlet.
> >
> > - Henry
> >
> >
> > On 2010/07/22 23:28:45, johnfargo wrote:
> >
> >> http://codereview.appspot.com/1665054/diff/1/2
> >> File
> >>
> >
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java
> >
> >> (right):
> >>
> >
> >  http://codereview.appspot.com/1665054/diff/1/2#newcode125
> >>
> >
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetRenderingServlet.java:125:
> >
> >> UriStatus urlstatus, GadgetContext context, RenderingResults results)
> >>
> > throws
> >
> >> IOException {
> >> I'm not opposed, but do cast a somewhat suspicious eye on a catch-all
> >> postprocessing method that has a kitchen sink of arguments passed to
> >>
> > it :) I'm
> >
> >> wondering if we can make the types of customization more precise, to
> >>
> > encourage
> >
> >> reuse of common code.
> >>
> >
> >  Do you have any examples (even if abstract) of the types of processing
> >>
> > desired?
> >
> >  What's been moved into this method is fairly generic stuff: cache
> >>
> > handling,
> >
> >> error handling, redirect handling. Do you want to customize each such
> >> subroutine? Add arbitrary content manipulation @ the end of this whole
> >>
> > block?
> >
> >
> >
> > http://codereview.appspot.com/1665054/show
> >
>

[henry.saputra, 2010-07-24 00:50:57]

Address John's suggestion/concern, I have separated the overrideable/protected
methods for each RenderingResults.Status enum value. 
This approach should make the hook methods to have more specific purpose.

The goal for this CL is to make developers life easier when they want to
customize default behavior in Shindig.

Comments/suggestions are welcomed.

- Henry

[Paul Lindner, 2010-07-24 00:54:18]

lgtm

[johnfargo, 2010-07-24 00:54:53]

lgtm also

On Fri, Jul 23, 2010 at 5:54 PM, <lindner@inuus.com> wrote:

> lgtm
>
>
> http://codereview.appspot.com/1665054/show
>