# Checking the code of XMage, and why you won't be able to get the special rare cards of the Dragon's Maze collection XMage is a client\-server application for playing Magic: The Gathering \(MTG\)\. XMage's development was started in early 2010\. Since then, it has seen 182 releases, attracted an army of contributors, and it's still being actively developed even now\. All that makes it a good reason for us to contribute to its development too\! So, today the PVS\-Studio unicorn is going to check the code base of XMage and maybe even get into a fight with some entities\. ![0758_XMage/image1.png](https://import.viva64.com/docx/blog/0758_XMage/image1.png) ## About the project [XMage](https://github.com/magefree/mage) is an actively developing application that has been around for 10 years already\. Its goal is to make a free, open\-source, online playable computer game version of the original [Magic: the Gathering](https://magic.wizards.com/en) card game\. Its features include: * access to about 19,000 unique cards released over the 20\-year history of MTG; * automatic control and full enforcement of all existing game rules; * multiplayer mode for playing against human opponents on public servers; * single\-player mode for playing against AI opponents; * dozens of formats and modes \(Standard, Modern, Vintage, Commander, and many more\); * support of both single matches and tournaments\. ## A brief digression I've stumbled across a 2018 [work](https://delftswa.gitbooks.io/desosa2018/content/xmage/chapter.html) made by students of Delft University of Technology \([Software Architecture](https://se.ewi.tudelft.nl/delftswa/) graduate master\-level course\)\. They were participating in active development of rather complex open\-source projects\. Over eight weeks, they were taking the course and studying those open\-source projects to understand and then describe the architecture of the software application of their choice\. They picked the XMage project, and one of the aspects of their work was to gather a number of metrics using SonarQube \(LOC count, cyclomatic complexity, code duplicates, code smells, bugs, vulnerabilities, etc\.\)\. What attracted my attention was the fact that in 2018, SonarQube had detected 700 bugs and vulnerabilities on the total of 1,000,000 lines of code\. I've looked through the history of contributions and discovered that based on that analysis report, a pull\-request was made for fixing about 30 defects in the categories "Blocker" or "Critical"\. What happened to the rest of the warnings is unknown, but I hope they weren't just left unaddressed\. That was two years ago; the code base has gained about 250,000 LOC more since then, which makes it an interesting idea to take a look into it and see how it's doing\. ## About the check The check was done on the XMage [1\.4\.44V0](https://github.com/magefree/mage/tree/xmage_1.4.44V0) release\. It's always nice to have a project which is able to be built with Maven without any issues \(just as described in the documentation\): ```cpp mvn clean install -DskipTests ``` No other actions were required of me\. Isn't that cool? Neither did I have any problems integrating the PVS\-Studio plugin into Maven – again, just as described in the [documentation](https://pvs-studio.com/en/docs/manual/6703/)\. The check finished with 911 warnings, 674 out of which were of the first and second levels\. I left out the third\-level warnings for this article since many of them usually turn out to be false positives\. However, don't ignore third\-level warnings in "actual combat" because some of them may point at critical defects too\. Besides, I didn't include a number of warnings produced by some of the diagnostics because I believe they should be checked by those who are more familiar with the project than I am: * V6022, which detects unused parameters in methods/constructors\. These make a total of 336 warnings, which is quite a lot\. * V6014, which detects situations when all exiting points of a method return the same value – 73 warnings\. * V6021, which detects situations when some result is stored to a variable and that variable is never used afterward – 36 warnings\. * V6048, which advises simplifying an expression – 17 warnings\. Add to that about 20 similar warnings produced by several diagnostics, which were clearly false positives\. Those we have put on our own todo list\! All in all, leaving out the ones mentioned above, I was left with about 190 warnings to examine\. Looking through them, I found lots of similar minor defects – either debug\-specific or related to meaningless checks and operations\. A lot of warnings were issued on one exceptionally strange code fragment asking to be refactored\. Out of those, I picked 11 diagnostic rules and one most interesting example to illustrate each\. Let's take a look at them\. **Warning 1** [V6003](https://pvs-studio.com/en/docs/warnings/v6003/) The use of 'if \(card \!\= null\) \{\.\.\.\} else if \(card \!\= null\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. TorrentialGearhulk\.java\(90\), TorrentialGearhulk\.java\(102\) ```cpp @Override public boolean apply(Game game, Ability source) { .... Card card = game.getCard(....); if (card != null) { .... } else if (card != null) { .... } .... } ``` This one is simple: the body of the second conditional statement _if \(card \!\= null\)_ in the _if\-else\-if_ construct will never execute because either execution won't reach this spot or _card \!\= null_ will always be _false_\. **Warning 2** [V6004](https://pvs-studio.com/en/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. AsThoughEffectImpl\.java\(35\), AsThoughEffectImpl\.java\(37\) ```cpp @Override public boolean applies(....) { // affectedControllerId = player to check if (getAsThoughEffectType().equals(AsThoughEffectType.LOOK_AT_FACE_DOWN)) { return applies(objectId, source, playerId, game); } else { return applies(objectId, source, playerId, game); } } ``` This is a plain mistake that I see quite often in open\-source projects\. Is this copy\-paste? Or am I missing something? My guess is that the _else_ branch should be returning _false_\. P\.S\. Just in case you may be wondering, there are no recursive calls of _applies\(\.\.\.\.\)_ here since these are different methods\. A similar warning: * V6004 The 'then' statement is equivalent to the 'else' statement\. GuiDisplayUtil\.java\(194\), GuiDisplayUtil\.java\(198\) **Warning 3** [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'filter\.getMessage\(\)\.toLowerCase\(Locale\.ENGLISH\)\.startsWith\("Each "\)' is always false\. SetPowerToughnessAllEffect\.java\(107\) ```cpp @Override public String getText(Mode mode) { StringBuilder sb = new StringBuilder(); .... if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) { sb.append(" has base power and toughness "); } else { sb.append(" have base power and toughness "); } .... return sb.toString(); } ``` [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) warnings are pretty common in each project we've checked, and XMage is no exception \(79 warnings\)\. All of them are generally correct, but many are specific only to debugging mode or point at spots where the developers chose to play safe, and so on\. It's usually better to leave such warnings to the project authors to investigate\. However, this particular one is definitely an error\. Depending on the beginning of the _filter\.getMessage\(\)_ string, either the string " has \.\.\." or " have \.\.\." is to be appended to _sb_\. The problem is that the developer is checking if the string starts with an upper\-case letter while having that same string converted to lower\-case before the check\. Oops\. This will result in appending the " have \.\.\." string every time\. The effect of this bug isn't drastic, but it's still unpleasant as it leads to incorrect grammar somewhere in the game\. Other warnings of this type that I found worth checking: * V6007 Expression 't\.startsWith\("\-"\)' is always false\. BoostSourceEffect\.java\(103\) * V6007 Expression 'setNames\.isEmpty\(\)' is always false\. DownloadPicturesService\.java\(300\) * V6007 Expression 'existingBucketName \=\= null' is always false\. S3Uploader\.java\(23\) * V6007 Expression '\!lastRule\.endsWith\("\."\)' is always true\. Effects\.java\(76\) * V6007 Expression 'subtypesToIgnore::contains' is always false\. VerifyCardDataTest\.java\(893\) * V6007 Expression 'notStartedTables \=\= 1' is always false\. MageServerImpl\.java\(1330\) **Warning 4** [V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Null dereference of 'savedSpecialRares'\. DragonsMaze\.java\(230\) ```cpp public final class DragonsMaze extends ExpansionSet { .... private List savedSpecialRares = new ArrayList<>(); .... @Override public List getSpecialRare() { if (savedSpecialRares == null) { // <= CardCriteria criteria = new CardCriteria(); criteria.setCodes("GTC").name("Breeding Pool"); savedSpecialRares.addAll(....); // <= criteria = new CardCriteria(); criteria.setCodes("GTC").name("Godless Shrine"); savedSpecialRares.addAll(....); .... } return new ArrayList<>(savedSpecialRares); } } ``` The analyzer points out the null dereference of _savedSpecialRares_, which takes place when the collection is about to be filled for the first time\. The first explanation that comes to mind is that the developer simply wrote _savedSpecialRares \=\= null_ instead of _savedSpecialRares \!\= null _by mistake\. But in that case, an NPE could occur in the _ArrayList_ constructor when returning the collection from the method since the _savedSpecialRares \=\= null_ scenario is still possible\. Relying on the first intuitive solution to fix the code isn't a good idea\. After digging a bit deeper, I figured out that s_avedSpecialRares_ is defined as an empty collection right off at declaration and is not re\-assigned anywhere after that\. It means _savedSpecialRares _will never be _null_, so the null dereference pointed out by the analyzer will never take place as the collection will never be filled\. Therefore, the method will always return an empty collection\. P\.S\. This bug can be fixed by replacing _savedSpecialRares \=\= null_ with _savedSpecialRares\.isEmpty\(\)_\. P\.P\.S\. I'm afraid you won't be able to get the rare cards of the [Dragon's Maze](https://mtg.gamepedia.com/Dragon%27s_Maze) set in XMage for some time\. Another null dereference: * V6008 Null dereference of 'match'\. TableController\.java\(973\) **Warning 5** [V6012](https://pvs-studio.com/en/docs/warnings/v6012/) The '?:' operator, regardless of its conditional expression, always returns one and the same value 'table\.getCreateTime\(\)'\. TableManager\.java\(418\), TableManager\.java\(418\) ```cpp private void checkTableHealthState() { .... logger.debug(.... + formatter.format(table.getStartTime() == null ? table.getCreateTime() : table.getCreateTime()) + ....); .... } ``` The ternary operator _?:_ returns the same value no matter the result of the _table\.getStartTime\(\) \=\= null_ condition\. I guess we should blame autocompletion for that\. This is how it could be fixed: ```cpp private void checkTableHealthState() { .... logger.debug(.... + formatter.format(table.getStartTime() == null ? table.getCreateTime() : table.getStartTime()) + ....); .... } ``` **Warning 6** [V6026](https://pvs-studio.com/en/docs/warnings/v6026/) This value is already assigned to the 'this\.loseOther' variable\. BecomesCreatureTypeTargetEffect\.java\(54\) ```cpp public BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) { super(effect); this.subtypes.addAll(effect.subtypes); this.loseOther = effect.loseOther; this.loseOther = effect.loseOther; } ``` Two identical assignments in a row\. It looks as if the developer got carried away pressing the shortcuts and didn't notice duplicating a line\. But since _effect_ has a lot of fields, this snippet calls for a closer look\. **Warning 7** [V6036](https://pvs-studio.com/en/docs/warnings/v6036/) The value from the uninitialized 'selectUser' optional is used\. Session\.java\(227\) ```cpp public String connectUserHandling(String userName, String password) { .... if (!selectUser.isPresent()) { // user already exists selectUser = UserManager.instance.getUserByName(userName); if (selectUser.isPresent()) { User user = selectUser.get(); .... } } User user = selectUser.get(); // <= .... } ``` The warning seems to suggest that the _selectUser\.get\(\)_ method could throw a _NoSuchElementException\._ Let's take a closer look at that\. If we assume that the comment about _user_ already existing is true, no exception will be thrown: ```cpp .... if (!selectUser.isPresent()) { // user already exists .... } User user = selectUser.get() .... ``` In this case, execution won't enter the body of the conditional statement, and everything will be fine\. But then we may ask: why make a conditional statement with intricate logic if it never executes? Now, what if the comment is wrong? ```cpp .... if (!selectUser.isPresent()) { // user already exists selectUser = UserManager.instance.getUserByName(userName); if (selectUser.isPresent()) { .... } } User user = selectUser.get(); // <= .... ``` In that case, execution enters the body of the conditional statement and retrieves the user by calling the _getUserByName\(\)_ method\._ _That user is again checked for validity, which implies that _selectUser_ may be uninitialized\. There's no _else_ branch provided to handle that case, so we'll end up with a _NoSuchElementException_ in that line\. **Warning 8** [V6042](https://pvs-studio.com/en/docs/warnings/v6042/) The expression is checked for compatibility with type 'A' but is cast to type 'B'\. CheckBoxList\.java\(586\) ```cpp /** * sets the model - must be an instance of CheckBoxListModel * * @param model the model to use * @throws IllegalArgumentException if the model is not an instance of * CheckBoxListModel * @see CheckBoxListModel */ @Override public void setModel(ListModel model) { if (!(model instanceof CheckBoxListModel)) { if (model instanceof javax.swing.DefaultListModel) { super.setModel((CheckBoxListModel)model); // <= } else { throw new IllegalArgumentException( "Model must be an instance of CheckBoxListModel!"); } } else { super.setModel(model); } } ``` The author of this code messed it up a bit: they first check that _model_ is not _CheckBoxListModel_ but then explicitly cast the object to that very type anyway\. This will cause the _setModel_ method to throw a _ClassCastException_ once it gets to this spot\. The _CheckBoxList\.java_ file was added two years ago, and the bug is still there\. There seem to be no tests for incorrect parameters, nor does this method seem to actually work with objects of inappropriate types, which explains why the bug has survived for so long\. If someone relies on this method and reads the Javadoc comment, they will expect an _IllegalArgumentException_ rather than a _ClassCastException_\. I don't think anyone would consciously handle this exception, but you never know\. Based on the documentation, this is what I think the code should look like: ```cpp public void setModel(ListModel model) { if (!(model instanceof CheckBoxListModel)) { throw new IllegalArgumentException( "Model must be an instance of CheckBoxListModel!"); } else { super.setModel(model); } } ``` **Warning 9** [V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'player' reference was utilized before it was verified against null\. VigeanIntuition\.java\(79\), VigeanIntuition\.java\(78\) ```cpp @Override public boolean apply(Game game, Ability source) { MageObject sourceObject = game.getObject(source.getSourceId()); Player player = game.getPlayer(source.getControllerId()); Library library = player.getLibrary(); // <= if (player != null && sourceObject != null && library != null) { // <= .... } } ``` [V6060](https://pvs-studio.com/en/docs/warnings/v6060/) warnings indicate situations where an object is used before being checked for _null_\. You can often see such warnings mentioned in articles about checks of open\-source projects: they usually result from sloppy refactoring or changing methods' contracts\. If you look at the declaration of the _getPlayer\(\)_ method, the problem becomes clear: ```cpp // Result must be checked for null. // Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null) Player getPlayer(UUID playerId); ``` **Warning 10** [V6072](https://pvs-studio.com/en/docs/warnings/v6072/) Two similar code fragments were found\. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'\. SubTypeChangingEffectsTest\.java\(162\), SubTypeChangingEffectsTest\.java\(158\), SubTypeChangingEffectsTest\.java\(156\), SubTypeChangingEffectsTest\.java\(160\) ```cpp @Test public void testArcaneAdaptationGiveType() { addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); // Enchantment {2}{U} addCard(Zone.BATTLEFIELD, playerA, "Island", 3); addCard(Zone.HAND, playerA, "Silvercoat Lion"); addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion"); addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <= addCard(Zone.HAND, playerB, "Silvercoat Lion"); addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion"); addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <= .... for (Card card : playerB.getGraveyard().getCards(currentGame)) { if (card.isCreature()) { Assert.assertEquals(card.getName() + " should not have ORC type", false, card.getSubtype(currentGame).contains(SubType.ORC)); Assert.assertEquals(card.getName() + " should have CAT type", true, card.getSubtype(currentGame).contains(SubType.CAT)); } } } ``` The fact that this defect was found in tests may force you to underestimate its significance \("It's not a big deal with tests\."\) If so, I disagree\. Actually, tests play quite an important part in development \(though not as prominent as coding does\), and when bugs show up in the release, it's tests and testers that fingers are pointed at\. Flawed tests are worthless\. Then why have them? Why waste resources on them? The _testArcaneAdaptationGiveType\(\)_ method tests the "Arcane Adaptation" card\. Each player is dealt cards in several playing zones\. Thanks to copy\-paste, _playerA_ will get two "Silvercoat Lion" cards in the "Graveyard" zone, while _playerB_ will have none\. This is followed by some magic and then the actual testing happens in the end\. When it comes to testing _playerB_'s "graveyard" in the current deal, the test never enters the loop since the "graveyard" has been empty from the start\. I found this out using the good old _System\.out\.println\(\)_ method when running the test\. The fixed copy\-paste: ```cpp .... addCard(Zone.HAND, playerA, "Silvercoat Lion"); addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion"); addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <= addCard(Zone.HAND, playerB, "Silvercoat Lion"); addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion"); addCard(Zone.GRAVEYARD, playerB, "Silvercoat Lion"); // <= .... ``` Once I've fixed the code, the test was able to check the creatures in _playerB_'s "graveyard"\. Viva _System\.out\.println\(\)_\! Interestingly, the test was green both before and after the fix, which was great luck\. On the other hand, when you introduce any modifications that change execution logic, such test could do you a disservice by reporting a pass even with errors present\. A few more copy\-paste related defects of this type: * V6072 Two similar code fragments were found\. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'\. PaintersServantTest\.java\(33\), PaintersServantTest\.java\(29\), PaintersServantTest\.java\(27\), PaintersServantTest\.java\(31\) * V6072 Two similar code fragments were found\. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'\. SubTypeChangingEffectsTest\.java\(32\), SubTypeChangingEffectsTest\.java\(28\), SubTypeChangingEffectsTest\.java\(26\), SubTypeChangingEffectsTest\.java\(30\) **Warning 11** [V6086](https://pvs-studio.com/en/docs/warnings/v6086/) Suspicious code formatting\. 'else' keyword is probably missing\. DeckImporter\.java\(23\) ```cpp public static DeckImporter getDeckImporter(String file) { if (file == null) { return null; } if (file.toLowerCase(Locale.ENGLISH).endsWith("dec")) { // <= return new DecDeckImporter(); } else if (file.toLowerCase(Locale.ENGLISH).endsWith("mwdeck")) { return new MWSDeckImporter(); } else if (file.toLowerCase(Locale.ENGLISH).endsWith("txt")) { return new TxtDeckImporter(haveSideboardSection(file)); } .... else { return null; } } ``` The [V6086](https://pvs-studio.com/en/docs/warnings/v6086/) diagnostic rule detects incorrectly formatted _if\-else\-if_ constructs, with the _else_ keyword missing\. This is exactly what the snippet above demonstrates\. In this case, the inaccurate formatting won't do any harm thanks to the _return_ _null_ statement, but it's cool to find issues like that since you can never be sure\. Here's an example of how a missing _else_ leads to unexpected behavior: ```cpp public SomeType smtMethod(SomeType obj) { .... if (obj == null) { obj = getNewObject(); } if (obj.isSomeObject()) { // some logic } else if (obj.isOtherSomething()) { obj = calulateNewObject(obj); // some logic } .... else { // some logic } return obj; } ``` Now, if _obj \=\= null_, the object will be assigned some value and, because of the missing _else_, forced to go down the entire _if\-else\-if_ line for checking, while the original idea was to have the object returned right off\. ## Conclusion With this article about the results of checking the XMage project we have demonstrated yet one more time the abilities of modern static analyzers\. In modern development, they are becoming more and more a necessity as software's complexity keeps growing\. No matter how many releases, tests, and how much feedback you have, bugs will always find a way into your code base\. Why not put up another barrier to strengthen your defense system then? As you have seen, analyzers are subject to producing false positives \(PVS\-Studio Java being no exception\)\. They may result either from imperfect implementation of the analyzer's diagnostic rules or from overcomplicated code, which the analyzer has a difficult time figuring out\. Please be understanding about it and don't hesitate to [report](https://pvs-studio.com/en/about-feedback/) any false positives you encounter, and while they are waiting to be fixed, you can use one of the warning suppression [mechanisms](https://pvs-studio.com/en/docs/manual/6703/)\. Feel free to [download](https://pvs-studio.com/en/pvs-studio/download/) the analyzer and give it a try\.