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

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

Martin,

 

Your new patch was deemed invalid, I suppose because of whitespace differences, but it was easy enough to do manually.

It all seems to work now, the test set is still passed, so I have committed your changes into master.

I have also added the new tile 235 to TileDesigner, to prevent that its XML representation gets overwritten by future tile updates.

 

Erik.

 

From: Dr....@t-... [mailto:Dr....@t-...] 
Sent: Wednesday, January 25, 2012 10:48 AM
To: Rails Development
Subject: [Rails-devel] Fw: Re: Push Request : 1880 Code Update Part II

 

Hi Erik,

 

The Class rails.game.special.ExtraTileLay is currently not used. It should be removed from the data definition of the BCR.

Von: "Erik Vos" < <mailto:eri...@xs...> eri...@xs...>

An: <Dr....@t-...>, "'Development list for Rails: an 18xx game'" <rai...@li...>

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

Datum: Wed, 25 Jan 2012 00:09:53 +0100

 

 

Martin,

 

The code compiles but doesn’t run, because you refer to a class "rails.game.special.ExtraTileLay" that doesn’t exist. Not sure what your intention is here – I think we have discussed before how to enable the BCR laying two tiles. Is this just a leftover?

 

Erik.

 

From:  <mailto:Dr....@t-...> Dr....@t-...  <mailto:[mailto:Dr....@t-...]> [mailto:Dr....@t-...] 
Sent: Tuesday, January 24, 2012 11:28 PM
To: Development list for Rails: an 18xx game
Subject: Re: [Rails-devel] Push Request : 1880 Code Update Part II

 

Hi Erik,

 

you are welcome, sorry for the double work :).

 

Regards,

Martin

 

Von: "Erik Vos" < <mailto:eri...@xs...> eri...@xs...>

An: <Dr....@t-...>, "'Development list for Rails: an 18xx game'" <rai...@li...>

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

Datum: Tue, 24 Jan 2012 23:06:22 +0100

 

 

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:  <mailto:Dr....@t-...> Dr....@t-...  <mailto:[mailto: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