Re: Review of latest code

["Alexey Parshin" <alexeyp@gmail.com>]

2008/7/30 Ilya A. Volynets-Evenbakh <ilya@total-knowledge.com>

Alexey Parshin wrote:

2008/7/30 Ilya A. Volynets-Evenbakh <ilya@total-knowledge.com <mailto:ilya@total-knowledge.com>>

   I finally started reviewing code.
   First thing that caught my eye:

   HomeServlet.cpp:


   string csp = "HomeView.csp";
   ....
   RequestDispatcher* destination = request.getRequestDispatcher(csp);

   This contradicts tech spec:
   http://www.total-knowledge.com/wiki/index.php/UU_Templates
   ---------------------
   Template organization
   Template mappings
   Templates (CSP pages) will have identical names, and will be
   mapped to subdirectories named based on theme.


I need an example of the correct mapping. The current code wasn't changed since (in that respect) since Sergey developed it.

There can be more then one HomeView.csp
one will be default/HomeView.csp
another one tkn/HomeView.csp
etc.

default/... is guaranteed to be there - rest aren't.

User sets a preferred theme. That preference is used to get the full URL, which will be
<theme name>/<view name> if available, or default/<view name> if not.

This functionality should be abstracted into a member function in UUServlet class.

So, you want to have something like:
 
   string csp = fullThemePath() + "HomeView.csp";

?

   Now, HomeView.csp itself.
   Currently it's doing lot of data retrieval work from DB.
   This sort of conflicts with the original idea of having all data
   retrieval be done from a servlet,
   and passing the data as request attributes.
   (This, by the way, would prevent your exception handling problem
   we discussed on IRC).


This is Ok. I wouldn't worry too much about:

1) Performance: we need to retrieve these data one way or another. It doesn't matter where - the retrieval time is the same. Actually, generating data into the memory in the servlet and later retrieving it out of memory creates unnecessary delay, even not to significant.

 

Yes, this data will need to be retrieved, and storing it in the request is a bit of overhead.
Although, if we ever decide to add some Servlet-side caching of data, it'll be easier to do in
single servlet, rather then multiple views (remember: there will be more then one theme).

2) Code complexity. The current implementation is much cleaner since it doesn't have to store objects in the session or request in servlet in order to retrieve it later. It also has the advantage of retrieving data when and how we need 'em.

However, it needs to perform error checking, which should really be handled by servlet - so that data presence is guaranteed inside view.
And then, copy-paste - remember - we'll have more then one theme.

The downside of retrieving data in servlets is: If there are several ways to get to the same page, all the servlets redirecting to that page should update page data, isn't it? Then the code updating the data should be duplicated in these servlets.

It's impossible to have more then one servlet redirect to same view page (by design).

Well, when we send a request from servlet to some .csp - the request should already have the data, in order for csp to just display the data. If servlet sends this request to HomeView, it should store the data for HomeView in the request.

    /// Retrieve data
    ///  ...
    /// Send the request
    request.getRequestDispatcher(csp)->forward(request, response);
 
Now, the thing I'm trying to avoid is generating data separately from the view. I want to prevent any code duplication. If we retrieve data in servlet, and store it in the request - then, for instance, both LoginServlet and RegistrationServlet sending request to HomeView should retrive data for HomeView not to mention that any page sending request to HomeView should do the same.

BTW, that error handling is almost transparent. The 'CString errors;' declaration is in the Header.csp, the display of the error is in the Footer.csp. All that is required is to add ',errors' to the list of print() parameters. So, this complexity is gone.

So, IMHO performance or code complexity wise my approach is better. Do you have any other arguments? I'm afraid to miss something I don't understand yet.

--
Alexey Parshin,
http://www.sptk.net

--
Alexey Parshin,
http://www.sptk.net