[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mifos-developer
Subject:    [Mifos-developer] Review of code and design
From:       "James Dailey" <JDailey () gfusa ! org>
Date:       2006-06-29 4:07:29
Message-ID: 9DD845C1ED0D5D40B4B56DF5A4B1EB0E8B9E68 () gfmail ! gfusa ! org
[Download RAW message or body]

We still very much need more people contributing to the discussion about
code improvements, design concerns, and architecture.  In particular,
I'd like to call out a review that we had done a while ago and to which
the Aditi team has responded with some improvements.  I'd like them to
point out which areas they have addressed specifically and how to locate
those in the svn at java.net. 

So, Chris Brumme, who volunteered via his company, did a review of code
back in April and we are asking him to comment again on the changes
made.  Because he synched up to the code prior to march 5th, the code
was M1 code design. 

So many - but not all - of his comments actually served to validate our
move to the M2 coding approach.  He also raised a number of new issues.
The full review can be viewed in the folders
https://mifos.dev.java.net/servlets/ProjectDocumentList?folderID=5569&ex
pandFolder=5569&folderID=0 at java.net.  

In summary, he was complementary about the overall project while raising
specific issues: 

* Use of Remote Server Mode (RSM), particularly with respect to testing 
* Late binding issues 
* Error handling and being consistent 
* Threading and synchronization 
* Persistence data races and data consistency 
* Support of application as deployed - lack of utilities 
* Security issues 

Below are the notes from the meeting held with the Aditi Tech Leads on
the feedback.  I apologize for the delay in sending - this one got stuck
in the outbox drafts for too long. 

-----Original Message-----
From: Amit Srivastava
Sent: Tuesday, May 09, 2006 8:29 PM
To: Bharat Ahluwalia
Cc: Kiran Chakravarthy
Subject: Notes from meeting


Following are my notes from the meeting - 

1) Late Binding - echoes concerns raised by others volunteers earlier.
Removed in M2 and will be phased out. Mostly present in M1 style code
which is to be migrated to M2 style.

2) Error Handling - valid concerns. Some places it just logs and digests
the error. Most of the code rethrows the exception back up so that the
exception handler at the framework level can handle it. This was the
reason for putting root cause every time the exception is wrapped to
something else. There are places where misses have been pointed out. We
will take a look at exception blocks and figure out on a case by case
basis. This is followed more strictly in M2 code.

3) Synchronization - This is a valid concern and is priority. The static
fields which are mutable needs review. If possible, public access has to
be removed or the shared access to be synchronized. AuditConfiguration,
Logger etc is initialized by the plugin before the application is
available for use. Make the objects final wherever possible. We need to
do this review and should be half a day's effort.

4) ThreadLocal - only for hibernate and recommended by hibernate teams.
No other usage is seen in future. Chris agreed with this.

5) sql scripts - Was agreed that this could be a very useful tool.
Consistency check scripts are not present as of now. This was decided to
be added as one of the features and to be discussed further.

6) Data inconsistency - In M2 style all transactions (create/update) are
part of a single transaction. M1 code with the above example in
check/update kind of work is to be phased out and moved to M2 style. We
open a hibernate session, and follow the long application transaction
pattern. This has been discussed with William and posted earlier. We
open a session in a filter as soon as a user request comes in, work with
the objects within the open session to facilitate lazy loading and close
it at the BaseAction level just before sending the response back to the
client.

A further comment on James mail is that we use Transaction provided by
hibernate and not our own transaction object. Similarly transparent
write behind is hibernate's feature provided by default and is always
there. There is no business logic in hibernate files. Persistence files
now just deal with crud of entities and other queries. Question was
raised by James on whether this is the same problem as commented by
earlier on the usage of persistence layer separate from business layer.
We clarified that this was a different problem. 
[james: Clarification. Agree it is a different problem, I asked if one
could mask the other - can they confound each other? ] 

Also decided the need to monitor performance of these transactions
during performance testing and decide on optimizations then.

7) Logging - The M2 code tries to do this in a much more formalized
manner. With debug/info at start of methods and appropriate error/warn
messages on error conditions. The practice needs to be enforced on M1
objects when we do migration.

8) DAO and subtypes - The comment was on M1 style. M2 is different and
has a persistence layer which almost always refers to the base class for
common operations after performing some additional operations on the
passed object. Thus it modifies the base class behaviour but does not
break the base class pattern.

9) Security - searchString: The :searchString could be unsafe for
injection and we will perform the checks suggested. This would be done
at other places is code as well to ensure only alphanumerics are allowed
as the first level check. This is second in priority after the static
usage checks.

10) usage of short for boolean type properties - this was a database
problems where bit support was not provided by mysql 4.1. This has been
fixed in the latest version. However the M2 style objects have corrected
this problem where the internal representation is short but the public
methods exposed behave as if the property in question was a boolean.
Example of the same are, CustomerBO, SavingsBO etc


Thanks,
Amit




[prev in list] [next in list] [prev in thread] [next in thread]