Hello guys! :) Ok, let's see thoose implementation problems with the "Seasons unhardcoded!" patch.
> Specific problems: > - Turns are supposed to be simple. I really do not like having to store a > specification inside them. I think that would be better to supply when > deriving the turn season. I also thought this at first, but now i think it is MUCH better to store the specification in the Turn class. I have just made a version (in several hours of work) of what you say, just for your sake. Result Repositories/Branch: https://sourceforge.net/u/fenyo1/freecol/ci/MoreSeasonsV2/ https://github.com/fenyo1/FreeCol/tree/MoreSeasonsV2 This is working just like Version-1, i have tested. But please take a look at the code. Take a look at Turn.toString()! What is the problem with that? The problem is that toString() also NEEDS a specification, and if the class itself does not have it, that means toString() must acquire it as a parameter. And why is that a problem? Because toString() is defined in the Object class, and have to be overridden. As you can see i have made it @Deprecated (and of course i defined an overloaded version with one parameter), so the compiler will generate Warning every time it is used. Because every single place it is used, we need to append the specification parameter for toString(). And if it is still called from somewhere, it generates an exception. But there is a little problem with this. The compiler only generates Warning on the case of an explicit call of toString(). But Freecol code has a lot of points where the Turn.toString() is called implicitly. I mean this: logger.finest("Turn is now " + getTurn() + duration); This is an implicit conversion of Turn -> String. It is using Turn.toString() method. So there are a lot of points in Freecol code where an implicit Turn -> String conversion is done like this, and we even do not get any warning at these points. So if we want it to be perfect, we would have to go through the whole code of Freecol, to see if there is an implicit conversion is used or not. That is almost impossible. (how much lines do we have now?:) Of course these points of code will generate an exception to the log, and from that we can fix those points. And i have fixed about 4-5 such points with this method, by testing. But i really dubt i have tested every single line of the code, so god knows how many such places still exist in the code, which has to be fixed. But if we would use the first version, where we store the Specification inside the Turn, all of this is not a problem, we can safely use the implicit Turn -> String conversion. So that is why i think it is MUCH better to store the Specification in the Turn class. After all, it's just a pointer, which is 32 or 64 bits long, i think it worths it. Oh, and this is all the same with HistoryEvent class! Take a look at that, i had to make HistoryEvent.toString() @Deprecated. Same reason. > - I am pretty sure Messages.fmtMessage is duplicating what is done in > Messages.message. Not exactly. I presume you think of formatMessage(String fmtMessage). Messages.message searches a single identifier and translates it, but formatMessage searches for identifiers specified with % signs, and replaces all of them using Messages.message. Please take a look at data/mods/daySeasons/specification.xml. (that's one of the example mods) You can see season names like this: "%model.season.january.name% 31" If you would pass this to Messages.message it would return nothing, because there is no such id with that string. But formatMessage will translate it as "January 31", because it will call Messages.message with the string "model.season.january.name". > - We do not touch the messages files other than the base English one. Please > do not put other FreeColMessages_*.properties files in merge requests. Understandable. I have fixed this in both branches (V1, and V2). https://sourceforge.net/u/fenyo1/freecol/ci/MoreSeasons/ https://github.com/fenyo1/FreeCol/tree/MoreSeasons https://sourceforge.net/u/fenyo1/freecol/ci/MoreSeasonsV2/ https://github.com/fenyo1/FreeCol/tree/MoreSeasonsV2 So i really recommend Version-1 instead of Version-2. And i have tested it a lot, i think it is good to go. Regards, Fenyo
------------------------------------------------------------------------------
_______________________________________________ Freecol-developers mailing list Freecol-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/freecol-developers
