Re: [Rails-devel] Another Bug I have found, and worked out a fix for

[Erik V. <eri...@hc...>]

      Feel free to look over my constructors...and let me know if I have
mucked up something or actually made the code cleaner please.

OK, here we go. Much of what I'm going to say is probably a matter of taste
(or style), and our tastes apparently differ. Nothing wrong with that. But
I, for me, don't see much improvement.
 
1. Adding constructors with a GameManager parameter is pointless, as all
Start/Stock/OperatingRound instances are (or should be) created with
newInstance(), which uses the no-parameter constructors. That is why Round
has a setGameManager() method, to be called after creating of any round
(mostly from GameManager). Via GameManager this method (indirectly) imports
references to all static objects into the rounds (and those objects that
aren't included yet will be in the future). 
 
2. The special classes like ShareSellingRound are currently created in a
different way ( that may change in the future, should we ever need
game-specific versions). These classes can equally well call
setGameManager(gameManager), rather that super(gameManager), making these
one-parameter constructors completely unncecessary.
 
3. In Round(), setGameManager(null) is a no-op, why adding it?
 
4. Again probably a matter of taste. The following three statements are all
equivalent:
 
    StockRound() { super(); }
    StockRound() {}
    <empty>
 
because the compiler is so friendly to add all that is missing. In such
matters I tend to code minimally: leaving out what is automatic or obvious
(in pedantic mode I like to say that I apply Occam's razor; I admit that
existing empty constructors like OperatingRound() had escaped that razor so
far). You seem more inclined to code maximally here, including stuff that
will exist anyway (stating the obvious, as it were). I tend to take "cleaner
code" pretty literally: "less code".
 
5. I personally hate these aThis and theThat variable names; YMMV.
 
6. I'm no fan of Javadoc comments like "Constructor with no parameters, call
the super Class (Round's) Constructor with no parameters". Javadocs should
describe a method's purpose or usage rather than its contents.
 
On the positive side, your version of setGameManager() is better, and it has
also become clear that ShareSellingRound and TreasuryShareRound should call
this method rather than doing their own settings.
 
I hope I haven't disappointed you too much - it's not my intention to put
you off, but I do think that you have added unnecessary code.
 
Erik.