Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only skip giveUnitControl if the value is false or true #8023

Merged
merged 2 commits into from
Nov 1, 2020
Merged

Only skip giveUnitControl if the value is false or true #8023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02ce891
Only skip giveUnitControl if the value is false or true
trevan Oct 31, 2020
5ca0fbf
Ignore giveUnitControl if its value is null
trevan Nov 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions game-core/src/main/java/games/strategy/engine/data/gameparser/GameParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,16 +895,16 @@ private List<Tuple<String, String>> setOptions(
}
// decapitalize the property name for backwards compatibility
final String name = LegacyPropertyMapper.mapLegacyOptionName(decapitalize(option.getName()));
if (LegacyPropertyMapper.ignoreOptionName(name)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind explaining why this if block is moved down @trevan ?

If we are going to ignore an option, why should we execute option.getValue() or option.getCount(), wouldn't those just be dead stores?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it because I needed access to option.getValue() and I thought it would make more sense to use the local variable value that was going to be created vs passing in option.getValue(). But I can move it back up and pass in option.getValue().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local variable IMO is legacy, it's an unnecessary intermediate variable. This class in general followed a C++ pattern (if not older actually, maybe a C pattern or even Pascal) to declar all local variables at the beginning of a method. IMO all usages of 'value' probably should just get inlined.

I did not spot you were using the value variable. That answers my question.

continue;
}

if (name.isEmpty()) {
throw new GameParseException(
"Option name with zero length for attachment: " + attachment.getName());
}
final String value = option.getValue();
final String count = option.getCount();
if (LegacyPropertyMapper.ignoreOptionName(name, value)) {
continue;
}
final String countAndValue = (count != null && !count.isEmpty() ? count + ":" : "") + value;
if (containsEmptyForeachVariable(countAndValue, foreach)) {
continue; // Skip adding option if contains empty foreach variable
Expand Down
6 changes: 4 additions & 2 deletions game-core/src/main/java/games/strategy/engine/data/gameparser/LegacyPropertyMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ public String mapPropertyName(final String propertyName) {
return propertyName;
}

static boolean ignoreOptionName(final String name) {
return name.equalsIgnoreCase("takeUnitControl") || name.equalsIgnoreCase("giveUnitControl");
static boolean ignoreOptionName(final String name, final String value) {
return name.equalsIgnoreCase("takeUnitControl")
|| (name.equalsIgnoreCase("giveUnitControl")
&& (value.equals("false") || value.equals("true")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you positive that value could never be null here? I don't remember if we translate empty values to empty strings or null values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value is never checked for null in the caller before using (see line 908 in GameParser) so I don't think it would ever be null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we just had a NPE in GameParser, I would not presume it to be already correct. Another consideration, the 'value' is very context dependent, a DTD previously would have enforced values being provided, that is not necessarily guaranteed now.

I wonder if there is any type of test coverage around this at all that could/should be updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I may be wrong about the DTD previously requiring a specific value, I'm not sure if it can constrain attribute values vs just requiring them being there. Regardless, it is context dependent if we can expect a value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on the NPE. It will now skip giveUnitControl if the value is null as well as true or false.

There is some test coverage but it requires creating a brand new xml with these attributes in it. I'm looking at if there is a way to be able to create the xml in memory instead of committing the xml as a resource.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can get away from XMLs for testing, that would be excellent.

The decoupling of XML parsing from the semantic data parsing into GameData was meant to help with this.

One should be able to set up a org.triplea.map.data.elements.Game object and then pass that to GameParser and avoid having to set up an XML. The former is well tested for a variety of XML snippets, so we can be pretty sure how that works. It should have some existing tests that might be interesting to know if we null out empty values. Beyond that, sending GameParser a defined Game object hopefully can do the trick. GameParser is still very rough, so it might take some plumbing work to accept a Game object here. Let me know if that turns out to be the case, for now a manual verification is enough or you can go the extra mile if you feel it's worthwhile given the context acquisition cost you will have payed.

}
}