From Justin:

StFgtRawMaker:

-Change "PrepareEnvironment" -> "prepareEnvironment", "FillHits" -> "fillHits"

-->done

-What is the usefulness of "TStopwatch clock;" in Make()?

->removed

-In fillHits():

     -should masks of full rdo/arm/apv be hardcoded or better included in some DB masking table

-->good question, commented out the 2012 part, left in the parts that cannot change. Checks for data sanity is done in A2CMaker, based on DB

        -it should probably at the very least have some timestamp flag associated with it so only 2012 data needs to satisfy these conditions

     -Strip type should not be hardcoded, as this flag is needed to separate ZS and non-ZS data.  How will this be implemented?  Maybe you could have separate data banks in the DAQ file, like the BSMD?

 ---->to be addressed: removed strip type

From Tom:

Class: StFgtRawMaker
Files: StFgtRawMaker/StFgtRawMaker.h, StFgtRawMaker/StFgtRawMaker.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.

--->added private version

---> correction: omitted and added comment

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

--->added private version

---> correction: omitted and added comment

Method names: Mix of method name capitalisation convention e.g. FillHits() vs.
setFgtDb().

--> fixed

Field names: fgtDb should be changed to mFgtDb. Initialisation could move to
initialisation list.

--->fixed

Uses private/protected/public: Yes
Inlining: Fine, though definition of setFgtDb() could move out of the class body.
Documentation:
No external documentation provided. No comments in header explaining the purpose or
function of the code. No comments describing methods. Few comments interspersed in
source file.

--> added more documentation

Notes and suggestions:
• Replace <math.h> with <cmath>

• #includes for TStopwatch.h, StMessMgr.h, Stypes.h could be moved to the .cxx file.

-> fixed (removed stopwatch, moved the other ones)
• Is #include <TString.h> required?

->removed
• Do FillHits() and PrepareEnvironment() need to be public?

-> protected now
• What is the TStopwatch in Make() used for?

->removed
• Unnecessary semicolons after closing braces e.g. at end of method definitions, if{}
statements, ClassImp().

->removed
• Inconsistent indentation, especially in FillHists(), makes the code harder to read.

->re-indented
• The deliberate omission of an assignment operator and copy constructor should be
noted in a comment.

->added operators private
• Comments explaining each method (preferably Doxygen-style) would be useful

->
• Not a rule, but I think that typically the ROOT ClassImp macro is placed before the
class implementation rather than after (at least, this is where I look for it).

->we have it overall at the end.

• In FillHits(), has an unnecessary ʻthisʼ: while(this->GetNextDaqElement...

->removed
• Not sure if this is something thatʼs required by the STAR standards, but thereʼs no
check that the ʻnewʼ operations are successful when allocating memory (e.g. catching
bad_alloc exception).

-->>not required
• There is a check in Make() to ensure PrepareEnvironment() returns kStOK, but this is
the only possible value returned, even if an error is encountered.
PrepareEnvironment() could simply return bool, with false indicating a nonrecoverable
error.

 ->fixed, prepare Environment will return kStFatal on allocation errors. Even though we don't think that this is necessary

allocation errors throw exception

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

--> removed extra semicolons