rails-devel

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

 

 

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

[brett l. <bre...@gm...>]

I think it's because the patches are missing any mail header
information. "am" stands for "apply mailbox", which makes it
especially useful if you use a text-based mail program, like mutt or
pine.

"git apply" (which is what 'git am' calls) does seem to correctly
detect the patch.

---Brett.



On Tue, Jan 24, 2012 at 5:06 PM, Erik Vos <eri...@xs...> wrote:
> 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
>
>
>
>
>
>
> ------------------------------------------------------------------------------
> Keep Your Developer Skills Current with LearnDevNow!
> The most comprehensive online learning library for Microsoft developers
> is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
> Metro Style Apps, more. Free future releases when you subscribe now!
> http://p.sf.net/sfu/learndevnow-d2d
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel
>

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

[<Dr....@t-...>]

Hi Erik, 

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

	Regards, 

	Martin

	Von: "Erik Vos" <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: 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

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

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

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