Java-extras sub-project, vacate 'games.strategy.util' #4690
Conversation
(1) Added New sub-project 'java-extras' - Useful for something like 'Interruptibles' that is shared between sub-projects. (2) Move contents of 'games.strategy.util' to new packages: - Move most of 'game-core'/'games.strategy.util' to 'java-extras'/'org.triplea.java' - Move 'EventThreadJOptionPane.java' to 'swing-lib'/'org.triplea.swing' - Move remainder of 'game-core'/'games.strategy.util' to 'game-core'/'org.triplea.util' (3) Begin breaking apart/removing 'Util.java': - Move sha512 method from 'Util.java' to the one class that used it, RSAAuthenticator - Deprecate 'not' predicate and create new 'NegationPredicate.java' to take place - Mark remaining Util.java methods with TODO for move
Codecov Report
@@ Coverage Diff @@
## master #4690 +/- ##
============================================
- Coverage 23.22% 23.16% -0.06%
+ Complexity 6213 6189 -24
============================================
Files 873 874 +1
Lines 70483 70487 +4
Branches 11242 11242
============================================
- Hits 16369 16328 -41
- Misses 52078 52126 +48
+ Partials 2036 2033 -3
Continue to review full report at Codecov.
|
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| import static com.google.common.base.Preconditions.checkNotNull; | |||
|
|
|||
| import games.strategy.util.Version; | |||
| import org.triplea.util.Version; | |||
There was a problem hiding this comment.
Why does this class even exist? It looks like it should get merged into Version.
There was a problem hiding this comment.
My guess is we have it to deal with when we had 5 version numbers but kept Version.java unchanged for compatibility. If so, that would make it legacy. For the purpose of evacuating games.strategy.util, it is one more thing to move.
There was a problem hiding this comment.
GameEngineVersion was introduced because Version is used for "versions" other than the game engine version. The semantics of what constitutes a compatible engine or map version with another engine version are specific enough that they probably shouldn't be in the same general type that is also used for things like lobby version, etc.
However, that may no longer be the case since, with the recent changes to Version's implementation, it's becoming less generic. In which case, anything else that happens to use it more like a general version type probably should be refactored to use a different type to avoid the coupling to what is effectively no longer a general utility class but rather something domain-specific.
| @@ -1,4 +1,4 @@ | |||
| package games.strategy.util; | |||
| package org.triplea.util; | |||
There was a problem hiding this comment.
This change is problematic.
Even though we're in a breaking savegame release, the version class is special in the sense that it's the first object to be deserialized.
If the Version object is compatible, the engine proceeds deserializing the rest.
Currently because the fields were renamed, all of the version numbers are 0, which are incompatible and thus the engine doesn't proceed and tells the user the savegame is incompatible.
Because you moved this class though, deserializing will probably result in a ClassNotFoundException when attempting to deserialize Version objects instead of gracefully telling the user the savegame is incompatible.
There are 2 ways I could come up with just now to avoid this:
- The trivial solution which involves not moving this class at all (I don't think it really belongs there in the first place tbh)
- The non-trivial solution which involves changing the exception handler to just tell the user "you're using an incompatible version" when a
ClassNotFoundExceptionoccurs. This however has some limitations, when you try to load a newer version of thesavegame with an older version of the engine. When you chose this path though, you should probably just serialize a String representation of the version to be safe in future versions.
I must admit that I'm not sure if this object is being sent over the network. You'd have to check that first.
There was a problem hiding this comment.
Thanks for the considered feedback, on the two main topics of net and save compatibility:
Network compatibility
For network compatibility I believe we should be okay as we can better enforce that two game-clients will be on the same version. Notably after release we wouldn't let 1.9 and 1.10+ versions in the same lobby together. The version value in (latest_version.properties)[https://github.com/triplea-game/triplea/blob/master/latest_version.properties] is plain-text. This should allow clients to reliably determine which lobby to join.
Save-game compatibility
Good thinking to check the error handling. One thing to know/point out after reviewing the error handling, we wrap any 'ClassCastException' as an IOException and the error handling upstream is then the same: https://github.com/triplea-game/triplea/blob/master/game-core/src/main/java/games/strategy/engine/framework/GameDataManager.java#L97. Note that currently ErrorMessage only shows the log message, thus regardless of exception type we'll get the same error message (note, #4680 would change that and we'd have the exception message printed).
With that, I looked at the error handling by saving a game with latest master at v1.10 and then checked two cases:
- (1) Version in package:
org.triplea.util - (2) Version moved back to package
games.strategy.util.Version - (3) Updated Version number to v1.11
Results (1)
Get the following error message:
Results (2)
Bit more interesting with this dialog:
Clicking through to load anyways gives this dialog:
This result is a bit more interesting as the first error window messaging is seemingly incorrect - it's the save-game that is out of date and not the engine.
Results (3)
Get the same error message regardless of where Version.java is located:
|
|
||
| private Util() {} | ||
|
|
||
| /** | ||
| * allow multiple fully qualified email addresses separated by spaces, or a blank string. | ||
| */ | ||
| // TODO: move to an EmailUtil class or to Strings utility. |
There was a problem hiding this comment.
As a performance improvement we should consider pre-compiling this regex and potentiall simplifying it.
There was a problem hiding this comment.
There are some pretty interesting reads on the topic:
The net takeaway I took from the variety of sources was to just check the min length and an @ sign appears. That will validate most errors, regex updates beyond that hit diminishing returns and/or can make bad assumptions and be incorrect.
Whether we have a regex or not, the purpose is to remove this method from Util.java. I'm not sure anyone would know to really expect to find this here. Given the input is a string, it seems like a good thing for the Strings utility class, or alternatively we remove this utility method in favor of using a value object that would hold/carry a validated email address value.
I did start to break up Util.java, confronted with a number of much deeper questions like the above, and considering the file moves could easily start to become 'deleted' + 'new' files, I stopped breaking up Util.java and added these TODOs.
There was a problem hiding this comment.
While I agree that the regex is probably overkill and not perfect anyways, I'd just like to point out that this regex goes even further and is checking if the input string matches a list of emails for PbEM for example.
| * | ||
| * @return A predicate that represents the logical negation of the specified predicate. | ||
| */ | ||
| public static <T> Predicate<T> not(final Predicate<T> p) { |
There was a problem hiding this comment.
Do we really need a dedicated class to expose a single trivial method?
There was a problem hiding this comment.
I get the impression that you view a 'class' as a high cost. Is that the case and concern?
I'm reminded of an anecdote when Java 1.2 and 1.3 when classes were very expensive and clever tricks were used to avoid them (and to good effect). As an anecdote, I once checked Java perf comparing then to now, in Java 1.8 a no-op method was sometimes slower than a new object in Java 1.8. The difference was not quite statistically significant, surprising IMO to see at all.
If we are concerned about an explosion of many micro-classes, in this case the package scope is pretty well defined and I do not think we would actually have that problem until we added another dozen or more classes. We have some breathing room.
To another extent, the original authors/designers of the existing code decided already that the method should not be a local utility and had it extracted to its own stand-alone class, ie: Util.java. From that perspective the decision was already made to have this in its own class.
Util.java has a number of problems to it. @RoiEXLab would you agree we should try to deprecate Util.java and break-up and/or move its methods to more appropriate locations? Noting the TODOs on the Util.java, all of its methods are slated to move, it will no longer exist. At some point we could have the problem that this would be the only method on Util.java.
There was a problem hiding this comment.
I did think about this today and I came to the conclusion that my point of view is just based on the impression that splitting up content into multiple classes just seems like it turns everything into "more" code.
On the other hand I now think splitting up classes is a useful thing to do, when you keep class loading in mind. The more classes are split into logical groups, the less unecessary methods are being loaded.
So no objections.
…moving "Version.java"
| @@ -1 +1 @@ | |||
| version = 1.10.@buildId@ | |||
| version = 1.11.@buildId@ | |||
There was a problem hiding this comment.
Errm this isn't going to fix the problem I mentioned
Because we consider 1.10 to be still in development, we had breaking changes all over the place without having to increment the version number each time. So this isn't necessary.
What I wanted to express is that we should avoid Exception messages in the users face that just indicate that they are using the wrong version.
There was a problem hiding this comment.
Maybe a better way to understand the concern here is to try loading a 1.9 save game and observing the difference in behavior between this branch and master.
There was a problem hiding this comment.
Ya, I did not think there was any expectation for compatibility as well. See the other response for results when checking how the game handles the incompatibility scenario.
There was a problem hiding this comment.
tl;dr, moving the package of Version.java has the same effect as incrementing the version number: 1ffa2a8 added to keep same version number as we get the same effect anyways.
The testing of error handling seems to indicate the way to address this #4690 (comment), the good mention/concern over error handling, is to increment version number; keeping Version.java in same package arguably makes it worse as the game then 'thinks' it can load the save-game to then be doomed to fail.
There was a problem hiding this comment.
@ssoloff I checked the difference of moving Version.java from one package to another and it should be noted the exception message/type does not matter currently in the error display. Please see results mentioned in: #4690 (comment)
…y after moving "Version.java"" This reverts commit 943e0d9.
There was a problem hiding this comment.
@DanVanAtta I wanted to bump my question in #4690 (review)
Also no more "real objections":
I think the GameLoading code should be changed to handle ClassNotFoundExceptions differently, so that the users gets a better error message, and we don't have to deal with that in the future.
Also I'm not sure if the Version class belongs into the new subproject as long as it's getting serialized into the savegame.
|
|
I suspect that would be an improvement. I think because we store version in a java serialized form, that is the problem to begin with and anything else is potentially just a band-aid. So YMMV. The scope of this update is just a file re-org though, so even had I wanted to change that, it would have impeded the review and been a bad PR. |
We could get two artifacts from same project, would just need to vary which main method is invoked. Hopefully we could enable one to have extra images, in which case there would be a significant size difference. |
Overview
1+2 below are the main/significant changes, (3) sets a direction for breaking up one file: Util.java so eventually it can be re-grouped to other utility classes or local utilities.
Changes
(1) Added New sub-project 'java-extras'
(2) Move contents of 'games.strategy.util' to new packages:
(3) Begin breaking apart/removing 'Util.java':
RSAAuthenticator
take place
Functional Changes
Manual Testing Performed
Before & After Screen Shots
Before
After
Additional Review Notes
games.strategytoorg.triplea#4689 more broadly discusses how packages ingames.strategywill be relocated