General Comments

Introduction

First and foremost - we should thank Tracy and Leon for taking the time to prepare for this review.  The time spent now will certainly go a long way towards getting our data structures organized.  Note some of the following might be nit picking...  TkrRecon has a long history and certainly some classes are defined as they are for historical reasons - now would seem a good time to do some house cleaning so that all involved are happy little TkrRecon users.

General Comments

Naming

Consistency in naming (and using good names) will go a long way to making classes more usable.  First we should review what our standards are:
http://www-glast.slac.stanford.edu/software/CodeHowTo/codeStandards.html

Class Names

Avoid names like TkrTracks - use of a plural to describe a collection has caused us much trouble in the past - perhaps TrkTrackCol?

Also, use of such names should be consistent....rather than TkrCandidates - how about TkrCandidateCol?

TkrPatCandHit - seems to refer to clusters... why not name it TkrPatCandCluster?

Member variable names

Adhere to the rules in the coding conventions - specifically note the capitalization rules.

See TkrFitTrack.h line 88 m_Xgaps for example
To adhere to our guidelines it should be m_xGaps.

Method names

Our conventions state that all methods start with a lowercase letter.

See TkrRecInfo.h line 62 TrackPar method for example

writeOut

This type of method, which prints the contents of an object is very useful.  The name of such a method should be standardized across all data classes - not just within TkrRecon.  I am not sure I like the writeOut in particular...perhaps write or print..  but in any event, it would be useful for the DSTF to recommend a standard name to be used by all.

initialize methods

Similarly, TkrClusters contains a method called ini( ) - in other places in the code (outside of TkrRecon) we have used init( ) and initialize( ).  All are fine - but choosing a standard would be very helpful.  This is really a comment directed at the DSTF - rather than TkrRecon.

Get and Set routines

There are a number of places in the code where get is used..others where it is not - we have decided to use get.

see TkrRecInfo.h lines 41 and 44 - getQuality( ) and energy( )

typedefs for std::vectors

Standardize the naming.. in some places there are names like TkrFitTrackCol (TkrFitTrack.h) in others, CandTrkVector (in TkrPatCand.h).  In general, we do prefer the use of Col - this keeps the name independent of the implementation. 

One other comment - I am not sure we really want to use Trk and Tkr...I realize Trk probably refers to track..while Tkr refers to Tracker...but I think it makes life confusing...

Namespace use

Any TDS should have some namespace defined.  We will most likely want to mirror the class name in the PDS version of these TDS classes.  Choose some nice name..maybe TkrRecon...

enum usage

This is a matter of personal taste - but I think it is useful to explicitly define the values for all elements in an enum.  It makes the values clear...and avoids future readers of the code from scratching their heads and wondering if enum numbering starts at 0 or 1.

See TkrRecInfo.h line 38 enum TrackEnd for example.

Iterator Access

Some classes provide access to iterators that were defined for vectors - that's a nice thing to do!  The methods names are not consistent across the classes.

See TkrPatCand.h line 56 getCandHitPtr( )
and
TkrFitTrack.h line 68 hitIterConst( )

Documentation

There are a number of classes that still do not adhere to the code documentation recommendations.  Any classes that have any longevity should be properly documented - especially data classes that will be visible to users.

Also, can the documentation refer to a document that properly defines all of our favorite TKR terms?  layer, (bi-layer??), plane, cluster, hit, etc?

Indentation

There are a number of files whose contents are not indented consistently.  Not a big deal - but it would help in readability.

Test routine

Is there a test routine which will exercise the TkrRecon data classes?  This is separate from testing the algorithms.  It would be helpful to make sure that what you think you stored in the classes, actually comes out the other end.  This will also be vital for the PDS version of the TkrRecon classes.

Which Classes are destined for GlastEvent?

TkrTracks, TkrCandidates?
What about TkrFitTrack?
How about TkrFitPar and TkrFitMatrix?

Basically, any class moved to GlastEvent should not include header files from TkrRecon.  Is this possible?  What set of files will be moved?

Update GlastEventModel Page

Detailed Comments

TrkRecInfo
Update the Doxygen class comments to state that this is a pure abstract class - all of the methods must be defined by those classes are derived from it.

TkrCluster
Document what a flag is a little better - how is it used? 
Why do the size and strip methods return doubles and not ints?

TkrPatCand
First, what is the difference between TkrCluster and this class?  TkrPatCand just seems to contain TkrCluster data members.
The fact that the various methods derived from TkRecInfo take a TrackEnd parameter is a bit misleading.  These routines in TkrPatCand do not use that TrackEnd parameter at all.
See TkrPatCand.h line 27, the layer(TrackEnd end) method - returns the firstlayer.
Why does this class have a lastLayer( ) method - when it has a layer( ) method?  If you do keep lastLayer( ) it should probably be:
int lastLayer( ) const;

See line 53 getFoLPlane( ) - I really do not like the name of this method - why not call it plane(TrackEnd end)?  That would be consistent with the layer method name.