AI consider enemy turn order

[ron-murhammer]

[DanVanAtta]

Keep this as one method rather than splitting it up as two? I'm also unsure why the method would be part of the public API and not private, but re-merging it back to one would make that a moot point.

[ron-murhammer]

Agree. I ended up already doing this in the next set of changes. Really just did this to avoid having to update any references that called the original method without the extra parameter. I'd prefer to just merge this one as I have lots of refactoring changes already made in my next PR.

[DanVanAtta]

• There are some significant code quality issues that IMO need to be addressed before much else can be done. Essentially a few rounds of refactoring, adding unit/tregression tests, intermediary data structures and breaking up long methods.
• The one "key" new method added needs a unit test to make sure you covered your "Ps" and "Qs" and so we have a regression test in place
• I can't tell if strength of enemy players is considered or not when considering if an enemy can move over a newly captured territory. I also can't tell if allied counter attacks/counter captures are handled correctly. Essentially the question would be, if you had a stack of german units one space away from moscow, and you pull up one italian tank, would the Russian AI overreact thinking that it can't stop the can-opener? The answer to that question can change if UK is around for example to re-capture the territory.
• If last point is addressed, then sweet. Otherwise beyond the existing code quality issues this does look like a step in the right direction in terms of functionality.

[DanVanAtta]

" I'd prefer to just merge this one as I have lots of refactoring changes already made in my next PR."

If it helps and will make the review easier, then please go for it. Otherwise personally I would like to keep this open for a bit as a reminder to add the unit test for the new code.

[ron-murhammer]

• So both refactoring and unit tests are really lacking right now in the AI code. The biggest reason for this is the scope of what really needs done is pretty large and I really haven't had much time recently to code. I know its not a great excuse but I'd like to refactor significantly before writing unit tests for any AI code mostly just to save time (and yes I know one of the points of unit tests is to be able to refactor with lower risk of breaking functionality).
• So for this initial implementation, the AI is overly conservative with can-opener attacks. It essentially considers that even just one unit can clear any territory as you describe above and it doesn't rely on any of its allies to block since these then rely on what the enemies do (you get into starting to have to simulate out turns which given the complexity isn't possible with today's compute power). Its somewhat more complicated then you describe since the current enemy attack options are calculated before the AI makes its non-combat moves so it doesn't know how many units it has to block potential can-openers yet. It essentially would need to recalculate enemy attack options after it moves each of its units in order to consider if a territory can be blocked or not. This again would take a significant amount of processing power for probably somewhat minimal gains.

To summarize, the AI plays conservative around can-openers to avoid losing its key factories and capital with this initial implementation and there are definitely ways to improve it but processing time needs to be considered.

• So I think I'm going to merge this since I tested it and think the cod quality is probably about equal to the existing (so better functionality and equally poor code quality in the AI). I have a few more commits that I'll be making a PR for soon that add more turn order functionality but start tackling some of the refactoring.

[DanVanAtta]

unit tests are effectively regression tests. Personally I would take out abstract out chunks of code wholesale into new modules that have nice API's, test those, then reduce the code and refactor more. Repeating enough times and the original code gets to be higher level and dependent on only stuff that has been tested.

I've learned the hard way that you go faster when you clean the code first then do stuff. Working with dirty code and muddling through is just slower in the long run, generally hitting the few bugs and getting really slowed down by them can be the difference, and the worst case is more extreme.

Once code is written though IMO you lose quite a bit of value from tests. They are good for telling you when you are done. For example in above, a stub interface and then writing tests that fail against an empty implementation would be a good way to generate tests before the implementation was copied over.

*note: sorry if this is rough, didn't go through and edit it in favor of time.