[Jhs-leads] Zero Tolerance Project Reviews

[Kirkpatrick, Ivan]

For all Project Managers and Project Leads,

I am offering the following as help and assistance in developing your
applications.  Everything that follows should be considered as leading =
toward
Software Engineering Best Practices that hopefully, we are all engaged =
in.
Everything here is intended to promote and assist in developing good, =
high
quality, Object Oriented Designs and software that implements those =
designs.

The following notes are a sanitized version of an application review I
conducted recently.  I use sanitized to mean that the application is not
named explicitly.  This is considered part of the information and =
reviews
already documented at http://epic52/operations/J2EE-Gateways.html

We have all been working with the JHS long enough now to have a pretty =
good
understanding of how this system works.  After July we will have a zero =
or
near zero tolerance for applications which do not exhibit the desired =
degree
of code development and project management.

This means zero or near zero checkstyle errors on a daily basis.  Fixing =
up
all the checkstyle errors at the end of a development cycle is not what =
we
want to do.  We want all developers trained to write good code =
continuously.

We expect to see every java class that is possible to test with JUnit to =
have
an associated *Test.java JUnit test.  Insure that all of the test code =
is in
a test directory with a parallel package structure to the src directory.
Follow the naming convention for Test classes.  Each java class file =
should
have a corresponding *Test.java JUnit test class.  Classes that are not
testable outside the container do not require JUnit tests but are =
expected to
be tested inside the container. =20

JMeter tests should be developed for in container testing.  It is simple
enough to use and provides the ability to repeat tests accurately.

As soon as is possible we will implement daily monitoring and tracking =
of the
results of the maven builds and deployments.  We expect to see a =
complete and
accurate maven website however minimal it may be.  More information =
regarding
installation details like database ports, machines and firewall =
penetration
requirements are considered essential.  The link at
http://epic52.dep.state.fl.us/grp-proj/ will be checked for each project =
as
well as the link at http://epic52:7778/GrpProj .  An automated count and =
web
page display will show the results for each project.  Failures in this
respect show the java team and management (and everyone else that =
checks)
that the project has not fully embraced the continuous integration =
features
of the JHS system.

Beyond the mere existence of, and ability to build and deploy, the =
opening
paragraph at http://epic52.dep.state.fl.us/grp-proj/ should be updated.  =
The
project.xml file should contain information beyond what is required to =
get
the project to build and deploy using the Maven tools.  The opening =
paragraph
should be applicable to the true project purpose and intent.

Additional project information is easily included in the maven website.  =
This
could be done by editing the project.xml file and or by utilizing the =
xdocs
features to create and update the web pages that appear as links on the =
left
side of the maven web site page.

At the very least either update these pages or disable the inclusion of =
them
in the pages.  This is easily done by editing the site.xml page in the =
xdocs
directory.  If none of the project information links on the left side of =
the
main page have been updated to reflect any new documentation or =
references,
then we conclude the docs must be located elsewhere or are non-existent.

We expect to see something that indicates a code review has been
accomplished.  We would prefer to see comments within the code =
indicating
that the code was reviewed, when, by whom, the findings and what was =
done to
correct them.  Evidence in CVS change logs that show the code was =
updated to
reflect comments from the code review would also suffice if the change =
log
comments are useful and do not merely reflect the information already
captured by CVS.  Many of the files do not appear to include the =
LICENSE.txt
information.  This is can be added to the IDE configuration one time and =
then
it appears in all the code.  Each project should be utilizing the tools =
in a
manner that indicates understanding of the configuration features =
possible.
If your developers are continuously fighting the IDE or CVS then =
something is
not configured correctly.  Prepare your tools and keep them sharp.

Checkstyle reports:

Approximately half of the checkstyle errors reported can be fixed by =
simply
reformatting the code properly.  In 30 minutes I reduced the checkstyle
errors for a project of 51 java classes from somewhere around 4700 to =
2700.
JDeveloper requires a Jalopy plugin to be able to do this.  Other =
plugins may
be better for this function.  Eclipse and Netbeans both include this
functionality internally.  As previously mentioned, the best practice is =
to
do this continuously not as an after thought.  Another source of errors =
is
lack of javadocs.  Developers are going to write javadocs, they will be
accurate and they will be reviewed.  Good javadocs are essential to =
being
able to maintain the code after it is written. =20

Any project that does not have a single JUnit test is a problem.  At the =
very
least it demonstrates that the project.xml file is correctly referencing =
the
JUnit tests and directory.  JUnit tests are extremely helpful to =
developers
as they write the code.  Once Developers are adequately exposed to it, =
most
embrace the concept of continuous testing.  JUnit is an excellent way to =
go
about developing good Object Oriented skills and thinking.  I =
specifically
want to make sure that everyone understands that long methods are bad!
Methods in java classes should be focused and easily understood.
Exceptionally long methods indicate a lack of proper OO design and
implementation.

JMeter testing is very useful in examining the memory and jdbc =
connection
issues as noted later in this review.  A container restart, which is
automatic for each deployment resets the application's memory and jdbc
connections.  By comparing the startup conditions to those after the
application is exercised it is possible to infer that some problems may =
or
may not exist.  JMeter will be very useful when these investigations are
required.

All application could also be better by utilizing logging more =
effectively.
Some log output that appears to be inside a loop will result in less =
than
desirable output.  We will revise the logging output recommendation to =
enable
class, method and line numbers to be output as well in the near future.
Every application should be writing logs to
/u01/projects/logs/Log4JLogs/GrpProj.log.  The output will appear at
http://epic52.dep.state.fl.us:8000/app-logs/Log4JLogs/ .  This is easily =
set
in the log4j.properties file which must be included in the META-INF =
directory
of the ear file.  Once set it is not necessary to tinker with it.

The container logs GrpProj_opmn.log are shown at
http://epic52.dep.state.fl.us:8000/app-logs/Log4JLogs/ .  If these show
instances of Exceptions being thrown, such as:
 WARN - Unhandled Exception thrown: class java.lang.NullPointerException
 and
 05/06/22 13:47:19 java.lang.NullPointerException
 05/06/22 13:47:19 	at
com.evermind.server.rmi.RMIServer.run(RMIServer.java:471)
 05/06/22 13:47:19 	at
com.evermind.util.ReleasableResourcePooledExecutor$MyWorker.run(Releasabl=
eRes
ourcePooledExecutor.java:186)
05/06/22 13:47:19 	at java.lang.Thread.run(Thread.java:534)

then it is apparent that some method is returning null.  Although the =
above
might be caused by the SSO, it still should be a converted to a checked
exception.

Several instances of NullPointerException appear in the logs.  This =
indicates
the application is not correctly checking for this condition somewhere =
in the
code.  Note that the Code Review Checklist
(http://epic52/operations/CodeReview.html), specifically encourages
developers to test for returning null values from methods for exactly =
this
reason.

Using the OEM tool we can see the application's memory consumption and a =
few
other parameters.  For example one application is consuming 109.52 =
MBytes of
memory:

	GrpProj	Jun 23, 2005 7:15:26 PM	0.00	109.52
=09
	it also shows JDBC Usage as=20
Open JDBC Connections		1
Total JDBC Connections		51
Active Transactions		0
Transaction Commits		0
Transaction Rollbacks		0

I would question the value of the total JDBC Connections.  Is this set =
in the
data-sources.xml and if this is in fact the desired condition.

Most of the other applications only consume about 40-50 MBytes of memory =
so
we may want to investigate this condition and determine if it returns to =
a
lower value when the application is less active or if in fact the memory
continues to increase which would indicate a memory leak of some sort.

It would be interesting to find out what happens to the connections when =
or
if the database is shutdown and restarted.  Does the OC4J instance
reestablish the jdbc connections automatically?  We know that it can be
configured that way so we would expect to see some tests along those =
lines.

OEM also reports:

Response - Servlets and JSPs
Active Sessions		6
Active Requests		1
Request Processing Time (seconds)		0.01
Requests per Second		0.10

I realize that the OEM display is not available to everyone.  Please =
contact
me or one of the java team for details on the OEM for each application.  =
This
is part of getting the application ready for Pre-production testing.

Ivan S Kirkpatrick