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

[Mark S. <mar...@gm...>]

On Thu, Dec 4, 2008 at 3:54 PM, Erik Vos <eri...@hc...> wrote:

>
>       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.
>
>

Style/Taste of course is a large part of why I was tinkering here. And yes,
different people have different preferences


> 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).
>
>

The reason I add the constructors with the GameManager parameter is to allow
the sub-class constructors, like 'ShareSellingRound' (it already had a
constructor with parameters) can call the super-class constructor with the
GameManager parameter and let IT worry about doing the initialization. Yes,
they could call setGameManager, (as you point out below). But the purpose of
a constructor is to INITIALIZE Stuff. That is their ONLY Purpose. If you
hate constructors, which it appears you do, then you can toss my
constructors, and call the appropriate initialization routines.


> 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?
>

I added the call to setGameManager (null) in Round to allow for any other
initialization that setGameManager may want to do in the future, to keep the
initialization in one place. The current effect of calling setGameManager
with null changes nothing as the routine is currently written. But I hate to
NOT initialize stuff, nor do I like to use default values. If I have code
specifically to set the value to something, I know where it is set. But
again, this is style/taste. I can take it or leave it really.


>
> 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".
>

This is again some of my old-school programming training. Don't trust the
compiler to do this sort of work for you. If you specifically state it, and
be clear about it, the compile won't mess it up. By leaving it "empty" you
make an assumption. And the bug I found was based on your assumption that
the initialization always worked. and yet a step was missed.


>
>  5. I personally hate these aThis and theThat variable names; YMMV.
>

Here is one of my purely personaly taste items. I tend to write code with
the arguments comming in prefixed with 'a' and local method variable
prefixed with 't' to make it clear within the code where it came from. If
this is something that will get under your skin, then before I add my code
base, I will need to go through it and strip those prefixes out.


>
> 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.
>

Since this was my first true attempt to write Javadocs, I was taking a
guess. I will be flexible here, and see if I can do better in the future.


>
> 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.
>
>

At least I did something that you don't outright hate. :-) But the
setGameManager I did not alter in any way (the one in Round.java). I simply
had the constructors call it properly.

It all gets back to my initial point of which level of the class hierarchy
should initialize what parts.

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.
>
>

Disappointed? - No. as you stated, it is mostly Style/Taste. You and Brett
have been engineering most of this product so far, so my comming in needs to
be balanced with your coding style. I cannot be the maverick who comes in
and shoots holes in everything everywhere just because I carry a shotgun,
and think I know how to use it.

I do feel I have much to contribute still.

But be warned, I will be picking on other aspects of the implementation,
here and there. There is some items that I have seen that set my teeth on
edge... (for example 'counting the number of passes' to determine if a stock
round is over)


> Erik.
>
>
>
>
>
> ------------------------------------------------------------------------------
> SF.Net email is Sponsored by MIX09, March 18-20, 2009 in Las Vegas, Nevada.
> The future of the web can't happen without you.  Join us at MIX09 to help
> pave the way to the Next Web now. Learn more and register at
>
> http://ad.doubleclick.net/clk;208669438;13503038;i?http://2009.visitmix.com/
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel
>
>