OK. Thanks!
Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov
On Thu, Jul 17, 2014 at 5:53 PM, Andrea Del Bene <an.delb...@gmail.com>
wrote:
> I admit I've changed a very sensible part of the code :), however after
> working with it I realized that we could reduce its complexity removing a
> couple of nesting levels "merging" them in the outer if...else block. I
> think this improve code quality and its readability. Let me explain you
> what I did. The code was:
>
> if (bufferedResponse != null)
> {
> logger
> .warn("The Buffered response should be handled by
> BufferedResponseRequestHandler");
>
> // if there is saved response for this URL render it
> bufferedResponse.writeTo((WebResponse)requestCycle.getResponse());
> }
> else
> {
> if (shouldRenderPageAndWriteResponse(requestCycle,
> currentUrl, targetUrl)) //
> {
>
> BufferedWebResponse response = renderPage(currentUrl,
> requestCycle);
> if (response != null)
> {
> response.writeTo((WebResponse)requestCycle.getResponse());
>
> }
> }
> else
> {
> if (shouldRedirectToTargetUrl(requestCycle, currentUrl,
> targetUrl))
> {
> redirectTo(targetUrl, requestCycle);
>
>
> // note: if we had session here we would render the
> page to buffer and then
> // redirect to URL generated *after* page has been
> rendered (the statelessness
> // may change during render). this would save one
> redirect because now we have
> // to render to URL generated *before* page is
> rendered, render the page, get
> // URL after render and if the URL is different
> (meaning page is not stateless),
> // save the buffer and redirect again (which is pretty
> much what the next step
> // does)
> }
> else
> ......etc.....
>
> now is
>
>
> if (bufferedResponse != null)
> {
> logger
> .warn("The Buffered response should be handled by
> BufferedResponseRequestHandler");
>
> // if there is saved response for this URL render it
> bufferedResponse.writeTo((WebResponse)requestCycle.getResponse());
> }
> else if (shouldRenderPageAndWriteResponse(requestCycle,
> currentUrl, targetUrl))
> {
>
> BufferedWebResponse response = renderPage(currentUrl,
> requestCycle);
> if (response != null)
> {
> response.writeTo((WebResponse)requestCycle.getResponse());
>
> }
> }
> else if (shouldRedirectToTargetUrl(requestCycle, currentUrl,
> targetUrl))
> {
>
> redirectTo(targetUrl, requestCycle);
>
>
> // note: if we had session here we would render the page to
> buffer and then
> // redirect to URL generated *after* page has been rendered
> (the statelessness
> // may change during render). this would save one redirect
> because now we have
> // to render to URL generated *before* page is rendered,
> render the page, get
> // URL after render and if the URL is different (meaning page
> is not stateless),
> // save the buffer and redirect again (which is pretty much
> what the next step
> // does)
> }
> else
> {
>
> two of the 'if' statements next to an 'else' have been merged with this
> one.
>
>
>
>
> On 17/07/2014 16:26, Martin Grigorov wrote:
>
>> what is/was useless ?
>> now I am even more confused what your "beautification" did with this
>> really
>> important and fragile part of Wicket code :-)
>>
>> I'm not saying that the new code is badly formatted. I was asking whether
>> "beautification" really means "code formatting"
>>
>> Martin Grigorov
>> Wicket Training and Consulting
>> https://twitter.com/mtgrigorov
>>
>>
>> On Thu, Jul 17, 2014 at 5:10 PM, Andrea Del Bene <an.delb...@gmail.com>
>> wrote:
>>
>> Method respond had high level of "if...else" nesting, a couple of them
>>> useless. in which way the new code doesn't respect Wicket's code format?
>>> Maybe my IDE did something wrong.
>>>
>>> Hi Andrea,
>>>
>>>> On Wed, Jul 16, 2014 at 8:33 PM, <adelb...@apache.org> wrote:
>>>>
>>>> Repository: wicket
>>>>
>>>>> Updated Branches:
>>>>> refs/heads/master c24d830cd -> bb9c1044e
>>>>>
>>>>>
>>>>> Code beautifying: method WebPageRenderer.respond
>>>>>
>>>>> What exactly "beautifying" means ?
>>>>>
>>>> It is a bit hard to see what is the actual change. It doesn't look like
>>>> applying Wicket's code format.
>>>>
>>>>
>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo
>>>>
>>>>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/bb9c1044
>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/bb9c1044
>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/bb9c1044
>>>>>
>>>>> Branch: refs/heads/master
>>>>> Commit: bb9c1044e5f1ba8902aa69073ef9fb236802d277
>>>>> Parents: c24d830
>>>>> Author: andrea del bene <adelb...@apache.org>
>>>>> Authored: Wed Jul 16 19:33:16 2014 +0200
>>>>> Committer: andrea del bene <adelb...@apache.org>
>>>>> Committed: Wed Jul 16 19:33:16 2014 +0200
>>>>>
>>>>> ----------------------------------------------------------------------
>>>>> .../request/handler/render/WebPageRenderer.java | 169
>>>>> +++++++++----------
>>>>> 1 file changed, 82 insertions(+), 87 deletions(-)
>>>>> ----------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/
>>>>> bb9c1044/wicket-core/src/main/java/org/apache/wicket/
>>>>> request/handler/render/WebPageRenderer.java
>>>>> ----------------------------------------------------------------------
>>>>> diff --git
>>>>> a/wicket-core/src/main/java/org/apache/wicket/request/handler/render/
>>>>> WebPageRenderer.java
>>>>> b/wicket-core/src/main/java/org/apache/wicket/request/handler/render/
>>>>> WebPageRenderer.java
>>>>> index 0b5dee4..af9dbee 100644
>>>>> ---
>>>>> a/wicket-core/src/main/java/org/apache/wicket/request/handler/render/
>>>>> WebPageRenderer.java
>>>>> +++
>>>>> b/wicket-core/src/main/java/org/apache/wicket/request/handler/render/
>>>>> WebPageRenderer.java
>>>>> @@ -198,99 +198,94 @@ public class WebPageRenderer extends PageRenderer
>>>>> // if there is saved response for this URL
>>>>> render
>>>>> it
>>>>>
>>>>> bufferedResponse.writeTo((WebResponse)requestCycle.getResponse());
>>>>> }
>>>>> + else if (shouldRenderPageAndWriteRespon
>>>>> se(requestCycle,
>>>>> currentUrl, targetUrl))
>>>>> + {
>>>>> + BufferedWebResponse response =
>>>>> renderPage(currentUrl, requestCycle);
>>>>> + if (response != null)
>>>>> + {
>>>>> +
>>>>> response.writeTo((WebResponse)requestCycle.getResponse());
>>>>> + }
>>>>> + }
>>>>> + else if (shouldRedirectToTargetUrl(requestCycle,
>>>>> currentUrl, targetUrl))
>>>>> + {
>>>>> +
>>>>> + redirectTo(targetUrl, requestCycle);
>>>>> +
>>>>> + // note: if we had session here we would render
>>>>> the page to buffer and then
>>>>> + // redirect to URL generated *after* page has
>>>>> been
>>>>> rendered (the statelessness
>>>>> + // may change during render). this would save
>>>>> one
>>>>> redirect because now we have
>>>>> + // to render to URL generated *before* page is
>>>>> rendered, render the page, get
>>>>> + // URL after render and if the URL is different
>>>>> (meaning page is not stateless),
>>>>> + // save the buffer and redirect again (which is
>>>>> pretty much what the next step
>>>>> + // does)
>>>>> + }
>>>>> else
>>>>> {
>>>>> - if (shouldRenderPageAndWriteRespon
>>>>> se(requestCycle,
>>>>> currentUrl, targetUrl)) //
>>>>> + if (isRedirectToBuffer() == false &&
>>>>> logger.isDebugEnabled())
>>>>> + {
>>>>> + String details = String
>>>>> + .format(
>>>>> + "redirect strategy:
>>>>> '%s',
>>>>> isAjax: '%s', redirect policy: '%s', "
>>>>> + + "current url:
>>>>> '%s', target url: '%s', is new: '%s', is stateless: '%s', is temporary:
>>>>> '%s'",
>>>>> +
>>>>> Application.get().getRequestCycleSettings().getRenderStrategy(),
>>>>> + isAjax(requestCycle),
>>>>> getRedirectPolicy(), currentUrl, targetUrl,
>>>>> + isNewPageInstance(),
>>>>> isPageStateless(), isSessionTemporary());
>>>>> + logger
>>>>> + .debug("Falling back to
>>>>> Redirect_To_Buffer render strategy because none of the conditions "
>>>>> + + "matched. Details: "
>>>>> +
>>>>> details);
>>>>> + }
>>>>> +
>>>>> + // force creation of possible stateful page to
>>>>> get
>>>>> the final target url
>>>>> + getPage();
>>>>> +
>>>>> + Url beforeRenderUrl =
>>>>> requestCycle.mapUrlFor(getRenderPageRequestHandler());
>>>>> +
>>>>> + // redirect to buffer
>>>>> + BufferedWebResponse response =
>>>>> renderPage(beforeRenderUrl, requestCycle);
>>>>> +
>>>>> + if (response == null)
>>>>> + {
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + // the url might have changed after page has
>>>>> been
>>>>> rendered (e.g. the
>>>>> + // stateless flag might have changed because
>>>>> stateful components
>>>>> + // were added)
>>>>> + final Url afterRenderUrl = requestCycle
>>>>> + .mapUrlFor(
>>>>> getRenderPageRequestHandler());
>>>>> +
>>>>> + if
>>>>> (beforeRenderUrl.getSegments().equals(afterRenderUrl.getSegments()) ==
>>>>> false)
>>>>> {
>>>>> - BufferedWebResponse response =
>>>>> renderPage(currentUrl, requestCycle);
>>>>> - if (response != null)
>>>>> - {
>>>>> -
>>>>> response.writeTo((WebResponse)requestCycle.getResponse());
>>>>> - }
>>>>> + // the amount of segments is different
>>>>> -
>>>>> generated relative URLs
>>>>> + // will not work, we need to rerender
>>>>> the
>>>>> page. This can happen
>>>>> + // with IRequestHandlers that produce
>>>>> different URLs with
>>>>> + // different amount of segments for
>>>>> stateless and stateful pages
>>>>> + response = renderPage(afterRenderUrl,
>>>>> requestCycle);
>>>>> + }
>>>>> +
>>>>> + if (currentUrl.equals(afterRenderUrl))
>>>>> + {
>>>>> + // no need to redirect when both urls
>>>>> are
>>>>> exactly the same
>>>>> +
>>>>> response.writeTo((WebResponse)requestCycle.getResponse());
>>>>> + }
>>>>> + // if page is still stateless after render
>>>>> + else if (isPageStateless() &&
>>>>> !enableRedirectForStatelessPage())
>>>>> + {
>>>>> + // we don't want the redirect to happen
>>>>> for stateless page
>>>>> + // example:
>>>>> + // when a normal mounted stateful page
>>>>> is
>>>>> hit at /mount/point
>>>>> + // wicket renders the page to buffer
>>>>> and
>>>>> redirects to /mount/point?12
>>>>> + // but for stateless page the redirect
>>>>> is
>>>>> not necessary
>>>>> + // also for listener interface on
>>>>> stateful
>>>>> page we want to redirect
>>>>> + // after the listener is invoked, but
>>>>> on
>>>>> stateless page the user
>>>>> + // must ask for redirect explicitly
>>>>> +
>>>>> response.writeTo((WebResponse)requestCycle.getResponse());
>>>>> }
>>>>> else
>>>>> {
>>>>> - if
>>>>> (shouldRedirectToTargetUrl(requestCycle, currentUrl, targetUrl))
>>>>> - {
>>>>> - redirectTo(targetUrl,
>>>>> requestCycle);
>>>>> -
>>>>> - // note: if we had session here
>>>>> we
>>>>> would render the page to buffer and then
>>>>> - // redirect to URL generated
>>>>> *after* page has been rendered (the statelessness
>>>>> - // may change during render).
>>>>> this
>>>>> would save one redirect because now we have
>>>>> - // to render to URL generated
>>>>> *before* page is rendered, render the page, get
>>>>> - // URL after render and if the
>>>>> URL
>>>>> is different (meaning page is not stateless),
>>>>> - // save the buffer and redirect
>>>>> again (which is pretty much what the next step
>>>>> - // does)
>>>>> - }
>>>>> - else
>>>>> - {
>>>>> - if (isRedirectToBuffer() ==
>>>>> false
>>>>> && logger.isDebugEnabled())
>>>>> - {
>>>>> - String details = String
>>>>> - .format(
>>>>> -
>>>>> "redirect
>>>>> strategy: '%s', isAjax: '%s', redirect policy: '%s', "
>>>>> -
>>>>> +
>>>>> "current url: '%s', target url: '%s', is new: '%s', is stateless: '%s',
>>>>> is
>>>>> temporary: '%s'",
>>>>> -
>>>>> Application.get().getRequestCycleSettings().getRenderStrategy(),
>>>>> -
>>>>> isAjax(requestCycle), getRedirectPolicy(), currentUrl, targetUrl,
>>>>> -
>>>>> isNewPageInstance(), isPageStateless(), isSessionTemporary());
>>>>> - logger
>>>>> - .debug("Falling
>>>>> back to Redirect_To_Buffer render strategy because none of the
>>>>> conditions "
>>>>> - +
>>>>> "matched. Details: " + details);
>>>>> - }
>>>>> -
>>>>> - // force creation of possible
>>>>> stateful page to get the final target url
>>>>> - getPage();
>>>>> -
>>>>> - Url beforeRenderUrl =
>>>>> requestCycle.mapUrlFor(getRenderPageRequestHandler());
>>>>> -
>>>>> - // redirect to buffer
>>>>> - BufferedWebResponse response =
>>>>> renderPage(beforeRenderUrl, requestCycle);
>>>>> -
>>>>> - if (response == null)
>>>>> - {
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> - // the url might have changed
>>>>> after page has been rendered (e.g. the
>>>>> - // stateless flag might have
>>>>> changed because stateful components
>>>>> - // were added)
>>>>> - final Url afterRenderUrl =
>>>>> requestCycle
>>>>> -
>>>>> .mapUrlFor(getRenderPageRequestHandler());
>>>>> -
>>>>> - if
>>>>> (beforeRenderUrl.getSegments().equals(afterRenderUrl.getSegments()) ==
>>>>> false)
>>>>> - {
>>>>> - // the amount of
>>>>> segments
>>>>> is different - generated relative URLs
>>>>> - // will not work, we
>>>>> need
>>>>> to rerender the page. This can happen
>>>>> - // with
>>>>> IRequestHandlers
>>>>> that produce different URLs with
>>>>> - // different amount of
>>>>> segments for stateless and stateful pages
>>>>> - response =
>>>>> renderPage(afterRenderUrl, requestCycle);
>>>>> - }
>>>>> -
>>>>> - if
>>>>> (currentUrl.equals(afterRenderUrl))
>>>>> - {
>>>>> - // no need to redirect
>>>>> when both urls are exactly the same
>>>>> -
>>>>> response.writeTo((WebResponse)requestCycle.getResponse());
>>>>> - }
>>>>> - // if page is still stateless
>>>>> after render
>>>>> - else if (isPageStateless() &&
>>>>> !enableRedirectForStatelessPage())
>>>>> - {
>>>>> - // we don't want the
>>>>> redirect to happen for stateless page
>>>>> - // example:
>>>>> - // when a normal
>>>>> mounted
>>>>> stateful page is hit at /mount/point
>>>>> - // wicket renders the
>>>>> page
>>>>> to buffer and redirects to /mount/point?12
>>>>> - // but for stateless
>>>>> page
>>>>> the redirect is not necessary
>>>>> - // also for listener
>>>>> interface on stateful page we want to redirect
>>>>> - // after the listener
>>>>> is
>>>>> invoked, but on stateless page the user
>>>>> - // must ask for
>>>>> redirect
>>>>> explicitly
>>>>> -
>>>>> response.writeTo((WebResponse)requestCycle.getResponse());
>>>>> - }
>>>>> - else
>>>>> - {
>>>>> -
>>>>> storeBufferedResponse(afterRenderUrl, response);
>>>>> -
>>>>> -
>>>>> redirectTo(afterRenderUrl,
>>>>> requestCycle);
>>>>> - }
>>>>> - }
>>>>> + storeBufferedResponse(afterRenderUrl,
>>>>> response);
>>>>> +
>>>>> + redirectTo(afterRenderUrl,
>>>>> requestCycle);
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>>
>
