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 <[email protected]>
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, <[email protected]> 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 <[email protected]>
Authored: Wed Jul 16 19:33:16 2014 +0200
Committer: andrea del bene <[email protected]>
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 (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
                  {
-                       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);
                          }
                  }
          }




Reply via email to