Re: [Rails-devel] Push Request : 1880 Code Update Part II

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

Thanks Martin, this is much better!

 

However, I’m having a problem to load these patches with ‘git am’, it says “Patch format detection fails”.  Does anyone have a clue?

 

Erik.

 

From: Dr....@t-... [mailto:Dr....@t-...] 
Sent: Tuesday, January 24, 2012 8:30 PM
To: Rails Development
Subject: [Rails-devel] Push Request : 1880 Code Update Part II

 

Hi Eric, et al.

 

please find attached the Code changes based on the originally submitted patch on Sunday.

 

Alternativly i have also attached the whole patch against the master also.

 

1.       Patch line 291: please don’t use static variables to represent dynamic and game-instance dependent data.  We want to be prepared to process multiple game instances in one program instance.

Fixed temporarly as i still need to find out how to access the instance of gameManager to get the variable to show (Lack of Java Experience on my side)

 

2.       Patch line 341: can’t the constant ‘CAPITALISE_FIFTY_PERCENT’ better be replaced by the more generic ‘CAPITALISE_PERCENTAGE’ with an additional numeric attribute that states the percentage? 

 

Took out that code entirely. Hardcoded the capitalisation in floatCompany of StartRound_1880 and StockRound_1880.

 

3.       I really wonder why you think you need an extra method PublicCompanyI.getNumberOfTileLays() with an additional company argument.  A company knows its own identity and name, doesn’t it?  So why pass that identity another time?

 

Lack of Java Experience, understood now :)

 

4.       Patch lines 416-421:  you shouldn’t include 1880-specific code into the generic  Round class the way you do here.  I don’t want to see references to 1880 and Shanghai in this class.   If you can’t find a way to reformulate this in a generic way, you can always override floatCompany() in  StockRound_1880.

 

See 2.

 

5.       Patch lines 644-648:  similar comment as point 2.  I believe we have discussed this before.

 

Fixed.

 

6.       BuyStartItem: why add a new boolean method isSharePriceToSet() if we already have the identical hasSharePriceToSet()?  And why can’t the whole Building Rights code be included in BuyStartItem_1880, which you have created anyway, instead of polluting BuyStartItem with that?

 

Incidentally that was a left over as the code had already been brought down to BuyStartItem_1880 and stemmed from some refactoring attempts that didnt get cleaned up, sorry about that.

 

7.       Similarly, why can’t the whole par slots code be included into StockMarket_1880?

 

Will be moved there.

 

And thanks again for your comments Erik :)

 

Kind Regards,

 

Martin