StFgtClusterMaker:

-It seems to me you should add the "default" cluster algo (whatever that

is) when this class is initialized, so there is always a cluster algo to be used even if its not added in the chain, is there a reason not to do this?  ie. add an else statement here (something like) if( !mClusterAlgoPtr ){ ... } else {

     LOG_INFO << "Using default fgt cluster algorithm" << endm;

     StFgtSimpleClusterAlgo* defaultAlgo = new StFgtSimpleClusterAlgo();

     setClusterAlgo(defaultAlgo);

}

--->to be discussed. This would introduce code dependencies as well: Added StFgtSeededClusterAlgo as default

 

 

-In documentation should also give some info on why there are 2 separate cluster algos?  Is one better than the other in general?  Which is the default?  Are there cases where the non-default is prefered?

--> added comment about default (now seededClusterAlgo)

 

-At the end of Make() need to remove all printf() and replace with probably LOG_DEBUG so don't get excessive strip/cluster info printed for every event in the standard chain...

 ----> done

 

Clustering Algos:

-StFgtSimpleClusterAlgo:

     -Probably needs more description on what steps actually are and what choices are made (both overview in header file and inline with code)

--->added comments/description

     -A few hard coded numbers eg. ordinate < 19 (line 106) and

numStrips<=10 (line 275) seem like fairly fundamental constants that might be better stored elsewhere?

 -->replaced by consts in StFgtConsts

-StFgtMaxClusterAlgo:

     -Probably neds more description on what steps actually are and what choices are made (both overview in header file and inline with code)

 

-->added comments/description

==>Default algo is now StFgtSeededClusterAlgo!

 

 

From Tom:

 

Class: StFgtIClusterAlgo
Files: StFgtClusterMaker/StFgtIClusterAlgo.h
Compiles: Yes
Constructor: N/A (abstract base class)
Destructor: No
Assignment operator: No. Indicate with comment that omission is deliberate, or make
it private if it shouldnʼt be accessible.
Copy constructor: No. Indicate with comment that omission is deliberate, or make it
private if it shouldnʼt be accessible.

-->made the comment

Method names: Yes
Field names: N/A
Uses private/protected/public: Yes
Inlining: N/A
Documentation:
No external documentation provided. Descriptions of the class and methods are OK, but
could be expanded (e.g. what is the return value of doClustering()?).
Notes and suggestions:
• There should be a virtual destructor.

-->added
• Unnecessary comma after ClassDef().
• For doClustering(), is StFgtStripCollection the output and StFgtHitCollection the
input? Or are they both input (the comment doesnʼt mention a strip collection). Can
the inputs be const references - presumably they shouldnʼt be modified?

-->see comments: StripCollection is input, but might be modified (strip status: Begin and end cluster) HitCollection is output
Class: StFgtSimpleClusterAlgo
Files: StFgtClusterMaker/StFgtSimpleClusterAlgo.h, StFgtClusterMaker/
StFgtSimpleClusterAlgo.cxx
Compiles: Yes
Constructor: Yes
Destructor: No. Add a virtual destructor so other classes can inherit from it.

-->done
Assignment operator: No. Indicate with comment that omission is deliberate, or make
it private if it shouldnʼt be accessible.

->done (added comment)
Copy constructor: No. Indicate with comment that omission is deliberate, or make it

->done (added comment)
private if it shouldnʼt be accessible.
Method names: Yes
Field names: Yes
Uses private/protected/public: Yes
Inlining: N/A
Documentation:
No external documentation provided. A more detailed description of the class in the
header would be desirable. The doClustering() algorithm needs to be described - aside
from what I can glean from reading the source code, I donʼt know what the clustering
algorithm does, so itʼs hard to give feedback on the implementation. There arenʼt many
comments explaining what is happening in the implementation file. Without a clearer
explanation of what is being done, I havenʼt given any suggestions for code
restructuring.

--->more comments added

Notes and suggestions:
• For the header, see comments for StFgtIClusterAlgo.
• Change <math.h> to <cmath>.
• mIsIntialized is set to true in Init(), then not used again. What is its purpose?

-->removed
• Use a STAR return code e.g. kStOK for the return value of Init().

-->done
• Some inconsistent indentation.
Class: StFgtMaxClusterAlgo
Files: StFgtClusterMaker/StFgtMaxClusterAlgo.h, StFgtMaxMaker/
StFgtMaxClusterAlgo.cxx
Compiles: Yes
Constructor: Yes
Destructor: No. Add a virtual destructor so other classes can inherit from it.

-->done
Assignment operator: No. Indicate with comment that omission is deliberate, or make
it private if it shouldnʼt be accessible.

--> added comment
Copy constructor: No. Indicate with comment that omission is deliberate, or make it
private if it shouldnʼt be accessible.

->added comment
Method names: Yes
Field names: Yes
Uses private/protected/public: Yes
Inlining: N/A
Documentation:
As for StFgtSimpleClusterAlgo.
Notes and suggestions:
• For the header, see comments for StFgtIClusterAlgo.
• mIsIntialized is set to true in Init(), then not used again. What is its purpose?

-->removed
Class: StFgtClusterMaker
Files: StFgtClusterMaker/StFgtClusterMaker.h, StFgtClusterMaker/
StFgtClusterMaker.cxx
Compiles: Yes
Constructor: Yes
Destructor: Yes
Assignment operator: No. Indicate with comment that omission is deliberate, or make
it private if it shouldnʼt be accessible.
Copy constructor: No. Indicate with comment that omission is deliberate, or make it
private if it shouldnʼt be accessible.

==>added comments
Method names: Yes
Field names: Yes
Uses private/protected/public: Yes
Inlining: N/A
Documentation:
As for StFgtSimpleClusterAlgo.
Notes and suggestions:
• Some very long lines; text wrapping makes reading hard.
• Make() is a little longer than I like for a single function; consider splitting it up to make
it easier to follow.
• Use StMessMgr instead of printf.

-->removed printf
• No check that StSPtrVecFgtHitIterator gives a valid pointer.

--> same as with new
Compilation warnings:
Numerous extra semicolons (compile with -pedantic to find them).

... decided not to go pedantic

.sl53_gcc432/obj/StRoot/StFgtClusterMaker/StFgtClusterMaker.cxx:14: warning:
unused parameter 'opts'

-->function not used right now: commented out