rails-devel

[Rails-devel] 1844 implementation and some general thoughts on Rails coding

[Stefan F. <ste...@we...>]

Martin:

now that I have hopefully all changes for 2.0 done, I will focus on 
1844/1854.

One thing I would like to avoid this time is the code duplication in 
sub-classes of major Rails classes (like OperatingRound, StockRound, 
PublicCompany etc.). This is a nightmare to support and debug.

What really makes this difficult is to find the differences to the 
standard implementation and checking if the difference is by chance 
(e.g. bug fixes not implemented there) or required due to a rule change.

Take for example StockRound:

In addition to the general class, there are 7 game specific sub-classes, 
4 more general sub-classes that itself contain another 5 game specific 
sub-classes.

If all of them copy the general code of share selling to change a minor 
detail how shares are sold, there are close to 20 implementations of 
share selling, which have to be maintained.

The general guidelines should be:

* Game specific code should only contain code that is relevant to 
implement a delta specific to that game. It should not copy general code.

* If this is not possible, comments on what was copied and what is the 
intended change is a MUST.

So Martin (and everyone else writing code for Rails) please think hard 
or ask for help BEFORE copying code from methods in general classes.

I know the problem is that many methods, especially in the Round 
classes, have been written in a fashion that reusing them is difficult 
and code copying easy.

However the better solution is: Refactor the general method FIRST to 
allow re-use and than call the re-factored method from the game specific 
class.

To increase co-operation for 1844 and 1854 development, I have added 
wiki pages for those games, that allow to share a list of things to do 
and what is already implemented.

http://github.com/freystef/Rails/wiki/1844
http://github.com/freystef/Rails/wiki/1854

Sorry for sounding harsh, that is not my intention. My intention is to 
make all our life in the future easier.

Stefan

Re: [Rails-devel] 1844 implementation and some general thoughts on Rails coding

[Martin B. <dr....@t-...>]

Am 30.08.2015 um 11:54 schrieb Stefan Frey:
>
> now that I have hopefully all changes for 2.0 done, I will focus on 
> 1844/1854.
>
> One thing I would like to avoid this time is the code duplication in 
> sub-classes of major Rails classes (like OperatingRound, StockRound, 
> PublicCompany etc.). This is a nightmare to support and debug.
>
Yes indeed :)
https://github.com/freystef/Rails/wiki/1844

I have added some points there for a start.

Regards,
Martin

Re: [Rails-devel] 1844 implementation and some general thoughts on Rails coding

[Stefan F. <ste...@we...>]

Martin:
the current state of mbr_2_1844 does not compile. After fixing the 
obvious typo there are even more errors.

This is in TrainManager_1844.

Some quick comments up-front:

* I do not think that we do not need special classes for Mountain or 
Tunnel companies. Most likely they will end up as PrivateCompany.
The only other idea what be to treat them as SpecialRight.

* The train conversion is mainly a change in the attributes for revenue 
calculation. Most likely there is no need to move certificates around, 
but merely some attribute state changes triggered by phase changes.
Other possibility is to use Double-Train Certificate, which Erik started 
to implement.

More to follow soon.

Stefan


On 08/30/2015 01:45 PM, Martin Brumm wrote:
> Am 30.08.2015 um 11:54 schrieb Stefan Frey:
>>
>> now that I have hopefully all changes for 2.0 done, I will focus on
>> 1844/1854.
>>
>> One thing I would like to avoid this time is the code duplication in
>> sub-classes of major Rails classes (like OperatingRound, StockRound,
>> PublicCompany etc.). This is a nightmare to support and debug.
>>
> Yes indeed :)
> https://github.com/freystef/Rails/wiki/1844
>
> I have added some points there for a start.
>
> Regards,
> Martin
>
>
>
> ------------------------------------------------------------------------------
>
>
>
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel
>

Re: [Rails-devel] 1844 implementation and some general thoughts on Rails coding

[Stefan F. <ste...@we...>]

Comments see below.

On 08/30/2015 06:27 PM, Martin Brumm wrote:
> Am 30.08.2015 um 17:51 schrieb Stefan Frey:
>> Martin:
>> the current state of mbr_2_1844 does not compile. After fixing the
>> obvious typo there are even more errors.
>>
>> This is in TrainManager_1844.
>>
>> Some quick comments up-front:
>>
>> * I do not think that we do not need special classes for Mountain or
>> Tunnel companies. Most likely they will end up as PrivateCompany.
>> The only other idea what be to treat them as SpecialRight.
>>
>> * The train conversion is mainly a change in the attributes for revenue
>> calculation. Most likely there is no need to move certificates around,
>> but merely some attribute state changes triggered by phase changes.
>> Other possibility is to use Double-Train Certificate, which Erik started
>> to implement.
>>
>> More to follow soon.
>>
>> Stefan
>>


> TrainManager_1844 is work in progress.
>
> Regarding the two special companies: yes i think they should be private
> companies or at least based upon that.
>
> Problems are that today Rails doesnt handle Private Companies in
> Stockrounds.. but we will find a solution there.
>

Yes a new action to buy private companies during stock rounds will be 
added. This allows to add the missing bit of implementation for 1830 as 
well, even if I play myself without that rule.

I will work on that part first: My intention is to break up StockRound 
into smaller bits that will be called "Activity" classes. Each 
"Activity" will be responsible for a single or a small set of actions.

So Round classes will be mainly containers for activity and control 
which activity will generate actions for what player/components.

There will be two (major) types of Round classes going forward: 
PlayerRound and CompanyRound.

So this is the start of some major break-up for the Round classes.


> I was dicussing the Approach with Erik and we both thought that the
> Double Train Certificate is the way to go there. See the Comment in
> TrainManager_1844. We probably only have to alter the Rusting Mechanismn
> there.
>

I know that Erik and mine approach differ in how directly we approach 
such an issue: I try to work around existing code, is Erik more used to 
attack the problem inside the core classes.

Or more precise: If I touch core classes, I try to do major changes in a 
single step to avoid touching them over and over again (see my break up 
of StockRound above).

So my approach would have been a less invasive and either add a Revenue 
modifier that treats the trains as H train after a given phase.

Or a phase trigger that swaps the train type of those train 
certificates. But I assume that is very close to what you are doing, 
however you (mis-)use the rusting mechanism as the phase trigger.

However I do not care how exactly things are done, if it works in the 
end and does not break existing games. ;-)