Example:  Geant4/examples/extended/persistency/P01/ 
All comments refer to:

geant 4 release: patch01 for Geant4 9.5 
platform: SLF 6 and UBUNTU

Reviewer: Hans Wenzel

1) Example compiles, runs (?)
   I got it to work only after I made some changes to
   the GNUMakefile and commenting out the call to G4Exception in 
   ExP01ChamberParameterisation.cc. I let Witold know about it
   it seems like the SVN tags got mixed up.
   Fixing the GNUmakefile probably doesn't make sense since we migrate to cmake.
   (so I would just get rid of it completely)
   Unfortunately the CMakeList.txt is not provided.
   In fact there is a CMakeList.txt file for P01 in SVN and it is tagged
   exampleP01-V09-05-03/ but for some reason it did not make it to the last
   reference release. It would be nice to add some instructions to the README
   file of the example how to build it using cmake:

   mkdir  /home/wenzel/P01-build
   cd /xxx/P01-build
   cmake -DCMAKE_MODULE_PATH=/xxx/geant4.9.5.p01/cmake/Modules \
         -DGeant4_DIR=/xxx/geant4.9.5.p01-install/lib/Geant4-9.5.1/ \
         /xxz/geant4.9.5.p01/examples/extended/persistency/P01

Using cmake the executable will not reside in $G4WORKDIR/bin/$G4SYSTEM/ anymore but 
in the /xxx/P01-build directory this should be reflected in the README file

2) External software documentation (if present)
May be the README file should point to the setup file of ROOT since this will 
setup ROOTSYS and the LD_LIBRARY_PATH
   source  /home/wenzel/root/bin/thisroot.sh


3) Classification of classes
   List classes specific to demonstrated features:
   
   ...   
   
   List of classes which can be replaced with common ones:
   
   ...
   
4) Messenger classes
   List commands defined in this example: 
   n/a (no messenger classes specifc to this example.). May be it would be nice to have a command to change the name of 
   the rot output file. 
   
   List commands which do not work as expected:
   a) not at all
   ...
   
   b) in some G4 phase(s)
   ...
   
5) Code review:
   Report on obsolete code (if present):
   ...
   
   Report on inefficient code (if present):
   ...
   
   Your recommendations to example owner:

   If I understand it correctly in the file ExP01TrackerSD.cc the lines below have the effect that an 
   static variable is HCID is set. So if the same sensitive detector class is used for a differnt volume 
   the additional sensitive detecor will never be initialized and added to the Hit collection. 

 static G4int HCID = -1;
 if(HCID<0)
    { HCID = G4SDManager::GetSDMpointer()->GetCollectionID(collectionName[0]); }
    HCE->AddHitsCollection( HCID, trackerCollection ); 
    }
 
 I would suggest to make HCID a member of the class instance    
 private:
    CalorimeterHitsCollection* calorimeterCollection;
    G4int HCID;

then in the constructor and Initialize method this variable would be set:

CalorimeterSD::CalorimeterSD(G4String name)
: G4VSensitiveDetector(name) {
    G4String HCname = name + "_HC";
    collectionName.insert(HCname);
    G4cout << collectionName.size() << "   CalorimeterSD name:  " << name << " collection Name: " << HCname << G4endl;
    HCID = -1;
}

//....oooOO0OOooo........oooOO0OOooo........oooOO0OOooo........oooOO0OOooo......

void CalorimeterSD::Initialize(G4HCofThisEvent* HCE) {

    calorimeterCollection = new CalorimeterHitsCollection
            (SensitiveDetectorName, collectionName[0]);
    if (HCID < 0) {
        G4cout << "CalorimeterSD::Initialize:  " << SensitiveDetectorName << "   " << collectionName[0] << G4endl;
        HCID = G4SDManager::GetSDMpointer()->GetCollectionID(collectionName[0]);

    }
    HCE->AddHitsCollection(HCID, calorimeterCollection);
}


this way each instance of the class will be initialized and given its HCID.

6) Documentation review:
   Using cmake the execuatble will not reside in $G4WORKDIR/bin/$G4SYSTEM/ anymore but 
   in the /xxx/P01-build directory this should be reflected in the README file

   May be the README file should point to the setup file of ROOT since this will 
   setup ROOTSYS and the LD_LIBRARY_PATH
   source  /home/wenzel/root/bin/thisroot.sh

7) Comment on the adequacy in its physics domain
   (skip this item if you are not an expert in physics demonstrated 
   in the example):
   ...    
   
8) "Users questions to be addressed in examples"   
   List questions which can be naturally addressed in this example:
   Very nice example how to provide persistency for the Hit collections created by Geant 4.    
   
   
Some remarks:
============
- didn't like the fact that when something didn't work
  (e.g. the dictionaries couldn't be loaded) the
  program just didn't write out anything without complaining. But that seems to be a problem of genreflex 
  in general not this example specifically. 

- the data in the root file is layed out weird. There is one object per event
  named event_"EVTNR" which is a vector of TrackerHits which is written out
  by the end of event action of the corresponding sensitive detector class.
  The event number is only a counter how often the RootIO routine was called. So if you write
  out more than one collection per event it would be called event_1 and event_2.
  I would propose a very different lay out:
  Write out a TTree where the leafs are events where each Event is a container
  containing an Event header and vectors of various Hits.
  I would propose that the write out is not initiated by the sensitive detector
  itself but it would be done by the end of general event action:
  this would create the event header then loop over all hit collections of all
  present sensitive detectors add them to the event and the write out the
  assembled event.
  I have implemented this suggestions my application which can be found here:
  http://home.fnal.gov/~wenzel/CaTS.html


- would be nice to provide an example root macro which demonsrates how to
  access the data from the root prompt. I send an example to Witold and will
  see if I can improve on it.