{"id":407715,"date":"2024-06-29T19:43:11","date_gmt":"2024-06-29T19:43:11","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=407715"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=407715","title":{"rendered":"<span>Checking the Code of XMage, and Why You Won&#8217;t Be Able to Get the Special Rare Cards of the Dragon&#8217;s Maze Collection<\/span>"},"content":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-1\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<div style=\"text-align:center;\"><img decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/getpro\/habr\/post_images\/ab5\/41b\/2b4\/ab541b2b4f379f0af908f736888b87f2.png\" alt=\"image1.png\" data-src=\"https:\/\/habrastorage.org\/getpro\/habr\/post_images\/ab5\/41b\/2b4\/ab541b2b4f379f0af908f736888b87f2.png\"\/><\/div>\n<p>  XMage is a client-server application for playing Magic: The Gathering (MTG). XMage&#8217;s development was started in early 2010. Since then, it has seen 182 releases, attracted an army of contributors, and it&#8217;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.<br \/>  <a name=\"habracut\"><\/a>  <\/p>\n<h2>About the project<\/h2>\n<p>  <a href=\"https:\/\/github.com\/magefree\/mage\">XMage<\/a> 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 <a href=\"https:\/\/magic.wizards.com\/\">Magic: the Gathering<\/a> card game.<\/p>\n<p>  Its features include:<\/p>\n<ul>\n<li>access to about 19,000 unique cards released over the 20-year history of MTG;<\/li>\n<li>automatic control and full enforcement of all existing game rules;<\/li>\n<li>multiplayer mode for playing against human opponents on public servers;<\/li>\n<li>single-player mode for playing against AI opponents;<\/li>\n<li>dozens of formats and modes (Standard, Modern, Vintage, Commander, and many more);<\/li>\n<li>support of both single matches and tournaments.<\/li>\n<\/ul>\n<p>  <\/p>\n<h2>A brief digression<\/h2>\n<p>  I&#8217;ve stumbled across a 2018 <a href=\"https:\/\/delftswa.gitbooks.io\/desosa2018\/content\/xmage\/chapter.html\">work<\/a> made by students of Delft University of Technology (<a href=\"https:\/\/se.ewi.tudelft.nl\/delftswa\/\">Software Architecture<\/a> 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.<\/p>\n<p>  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.).<\/p>\n<p>  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.<\/p>\n<p>  I&#8217;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 &#171;Blocker&#187; or &#171;Critical&#187;. What happened to the rest of the warnings is unknown, but I hope they weren&#8217;t just left unaddressed.<\/p>\n<p>  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&#8217;s doing.<\/p>\n<h2>About the check<\/h2>\n<p>  The check was done on the XMage <a href=\"https:\/\/github.com\/magefree\/mage\/tree\/xmage_1.4.44V0\">1.4.44V0<\/a> release.<\/p>\n<p>  It&#8217;s always nice to have a project which is able to be built with Maven without any issues (just as described in the documentation):<\/p>\n<pre><code class=\"java\">mvn clean install -DskipTests<\/code><\/pre>\n<p>  No other actions were required of me. Isn&#8217;t that cool?<\/p>\n<p>  Neither did I have any problems integrating the PVS-Studio plugin into Maven \u2013 again, just as described in the <a href=\"https:\/\/www.viva64.com\/en\/m\/0044\/\">documentation<\/a>. <\/p>\n<p>  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&#8217;t ignore third-level warnings in &#171;actual combat&#187; because some of them may point at critical defects too.<\/p>\n<p>  Besides, I didn&#8217;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:<\/p>\n<ul>\n<li>V6022, which detects unused parameters in methods\/constructors. These make a total of 336 warnings, which is quite a lot.<\/li>\n<li>V6014, which detects situations when all exiting points of a method return the same value \u2013 73 warnings.<\/li>\n<li>V6021, which detects situations when some result is stored to a variable and that variable is never used afterward \u2013 36 warnings.<\/li>\n<li>V6048, which advises simplifying an expression \u2013 17 warnings.<\/li>\n<\/ul>\n<p>  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! <\/p>\n<p>  All in all, leaving out the ones mentioned above, I was left with about 190 warnings to examine.<\/p>\n<p>  Looking through them, I found lots of similar minor defects \u2013 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.<\/p>\n<p>  Out of those, I picked 11 diagnostic rules and one most interesting example to illustrate each.<\/p>\n<p>  Let&#8217;s take a look at them.<\/p>\n<h2>Warning 1<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6003\/\">V6003<\/a> The use of &#8216;if (card != null) {&#8230;} else if (card != null) {&#8230;}&#8217; pattern was detected. There is a probability of logical error presence. TorrentialGearhulk.java(90), TorrentialGearhulk.java(102)<\/p>\n<pre><code class=\"java\">@Override public boolean apply(Game game, Ability source) {   ....   Card card = game.getCard(....);   if (card != null) {       ....   } else if (card != null) {       ....   }   .... }<\/code><\/pre>\n<p>  This one is simple: the body of the second conditional statement <i>if (card != null)<\/i> in the <i>if-else-if<\/i> construct will never execute because either execution won&#8217;t reach this spot or <i>card != null<\/i> will always be <i>false<\/i>.<\/p>\n<h2>Warning 2<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6004\/\">V6004<\/a> The &#8216;then&#8217; statement is equivalent to the &#8216;else&#8217; statement. AsThoughEffectImpl.java(35), AsThoughEffectImpl.java(37)<\/p>\n<pre><code class=\"java\">@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);   } }<\/code><\/pre>\n<p>  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 <i>else<\/i> branch should be returning <i>false<\/i>.<\/p>\n<p>  P.S. Just in case you may be wondering, there are no recursive calls of <i>applies(&#8230;.)<\/i> here since these are different methods.<\/p>\n<p>  A similar warning:<\/p>\n<ul>\n<li>V6004 The &#8216;then&#8217; statement is equivalent to the &#8216;else&#8217; statement. GuiDisplayUtil.java(194), GuiDisplayUtil.java(198)<\/li>\n<\/ul>\n<p>  <\/p>\n<h2>Warning 3<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6007\/\">V6007<\/a> Expression &#8216;filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith(&#171;Each &#171;)&#8217; is always false. SetPowerToughnessAllEffect.java(107)<\/p>\n<pre><code class=\"java\">@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(); }<\/code><\/pre>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6007\/\">V6007<\/a> warnings are pretty common in each project we&#8217;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&#8217;s usually better to leave such warnings to the project authors to investigate. <\/p>\n<p>  However, this particular one is definitely an error. Depending on the beginning of the <i>filter.getMessage()<\/i> string, either the string &#187; has &#8230;&#187; or &#187; have &#8230;&#187; is to be appended to <i>sb<\/i>. 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 &#187; have &#8230;&#187; string every time. The effect of this bug isn&#8217;t drastic, but it&#8217;s still unpleasant as it leads to incorrect grammar somewhere in the game.<\/p>\n<p>  Other warnings of this type that I found worth checking:<\/p>\n<ul>\n<li>V6007 Expression &#8216;t.startsWith(&#171;-&#171;)&#8217; is always false. BoostSourceEffect.java(103)<\/li>\n<li>V6007 Expression &#8216;setNames.isEmpty()&#8217; is always false. DownloadPicturesService.java(300)<\/li>\n<li>V6007 Expression &#8216;existingBucketName == null&#8217; is always false. S3Uploader.java(23)<\/li>\n<li>V6007 Expression &#8216;!lastRule.endsWith(&#171;.&#187;)&#8217; is always true. Effects.java(76)<\/li>\n<li>V6007 Expression &#8216;subtypesToIgnore::contains&#8217; is always false. VerifyCardDataTest.java(893)<\/li>\n<li>V6007 Expression &#8216;notStartedTables == 1&#8217; is always false. MageServerImpl.java(1330)<\/li>\n<\/ul>\n<p>  <\/p>\n<h2>Warning 4<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6008\/\">V6008<\/a> Null dereference of &#8216;savedSpecialRares&#8217;. DragonsMaze.java(230)<\/p>\n<pre><code class=\"java\">public final class DragonsMaze extends ExpansionSet {   ....   private List&lt;CardInfo> savedSpecialRares = new ArrayList&lt;>();   ....   @Override   public List&lt;CardInfo> getSpecialRare() {     if (savedSpecialRares == null) {                    \/\/ &lt;=       CardCriteria criteria = new CardCriteria();       criteria.setCodes(\"GTC\").name(\"Breeding Pool\");       savedSpecialRares.addAll(....);                   \/\/ &lt;=       criteria = new CardCriteria();       criteria.setCodes(\"GTC\").name(\"Godless Shrine\");       savedSpecialRares.addAll(....);       ....     }     return new ArrayList&lt;>(savedSpecialRares);   } }<\/code><\/pre>\n<p>  The analyzer points out the null dereference of <i>savedSpecialRares<\/i>, which takes place when the collection is about to be filled for the first time. <\/p>\n<p>  The first explanation that comes to mind is that the developer simply wrote <i>savedSpecialRares == null<\/i> instead of <i>savedSpecialRares != null <\/i>by mistake. But in that case, an NPE could occur in the <i>ArrayList<\/i> constructor when returning the collection from the method since the <i>savedSpecialRares == null<\/i> scenario is still possible. Relying on the first intuitive solution to fix the code isn&#8217;t a good idea. After digging a bit deeper, I figured out that s<i>avedSpecialRares<\/i> is defined as an empty collection right off at declaration and is not re-assigned anywhere after that. It means <i>savedSpecialRares <\/i>will never be <i>null<\/i>, 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>\n<p>  P.S. This bug can be fixed by replacing <i>savedSpecialRares == null<\/i> with <i>savedSpecialRares.isEmpty()<\/i>.<\/p>\n<p>  P.P.S. I&#8217;m afraid you won&#8217;t be able to get the rare cards of the <a href=\"https:\/\/mtg.gamepedia.com\/Dragon%27s_Maze\">Dragon&#8217;s Maze<\/a> set in XMage for some time.<\/p>\n<p>  Another null dereference:<\/p>\n<ul>\n<li>V6008 Null dereference of &#8216;match&#8217;. TableController.java(973)<\/li>\n<\/ul>\n<p>  <\/p>\n<h2>Warning 5<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6012\/\">V6012<\/a> The &#8216;?:&#8217; operator, regardless of its conditional expression, always returns one and the same value &#8216;table.getCreateTime()&#8217;. TableManager.java(418), TableManager.java(418)<\/p>\n<pre><code class=\"java\">private void checkTableHealthState() {   ....   logger.debug(.... + formatter.format(table.getStartTime() == null                                         ? table.getCreateTime()                                         : table.getCreateTime()) + ....);   .... }<\/code><\/pre>\n<p>  The ternary operator <i>?:<\/i> returns the same value no matter the result of the <i>table.getStartTime() == null<\/i> condition. I guess we should blame autocompletion for that. This is how it could be fixed:<\/p>\n<pre><code class=\"java\">private void checkTableHealthState() {   ....   logger.debug(.... + formatter.format(table.getStartTime() == null                                         ? table.getCreateTime()                                         : table.getStartTime()) + ....);   .... }<\/code><\/pre>\n<p>  <\/p>\n<h2>Warning 6<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6026\/\">V6026<\/a> This value is already assigned to the &#8216;this.loseOther&#8217; variable. BecomesCreatureTypeTargetEffect.java(54)<\/p>\n<pre><code class=\"java\">public BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {   super(effect);   this.subtypes.addAll(effect.subtypes);   this.loseOther = effect.loseOther;   this.loseOther = effect.loseOther; }<\/code><\/pre>\n<p>  Two identical assignments in a row. It looks as if the developer got carried away pressing the shortcuts and didn&#8217;t notice duplicating a line. But since <i>effect<\/i> has a lot of fields, this snippet calls for a closer look. <\/p>\n<h2>Warning 7<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6036\/\">V6036<\/a> The value from the uninitialized &#8216;selectUser&#8217; optional is used. Session.java(227)<\/p>\n<pre><code class=\"java\">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(); \/\/ &lt;=   .... }<\/code><\/pre>\n<p>  The warning seems to suggest that the <i>selectUser.get()<\/i> method could throw a <i>NoSuchElementException.<\/i><\/p>\n<p>  Let&#8217;s take a closer look at that.<\/p>\n<p>  If we assume that the comment about <i>user<\/i> already existing is true, no exception will be thrown:<\/p>\n<pre><code class=\"java\">.... if (!selectUser.isPresent()) {  \/\/ user already exists   .... } User user = selectUser.get() ....<\/code><\/pre>\n<p>  In this case, execution won&#8217;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?<\/p>\n<p>  Now, what if the comment is wrong? <\/p>\n<pre><code class=\"java\">.... if (!selectUser.isPresent()) {  \/\/ user already exists     selectUser = UserManager.instance.getUserByName(userName);     if (selectUser.isPresent()) {       ....     } } User user = selectUser.get(); \/\/ &lt;= ....<\/code><\/pre>\n<p>  In that case, execution enters the body of the conditional statement and retrieves the user by calling the <i>getUserByName()<\/i> method.That user is again checked for validity, which implies that <i>selectUser<\/i> may be uninitialized. There&#8217;s no <i>else<\/i> branch provided to handle that case, so we&#8217;ll end up with a <i>NoSuchElementException<\/i> in that line. <\/p>\n<h2>Warning 8<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6042\/\">V6042<\/a> The expression is checked for compatibility with type &#8216;A&#8217; but is cast to type &#8216;B&#8217;. CheckBoxList.java(586)<\/p>\n<pre><code class=\"java\">\/**  * 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);         \/\/ &lt;=     }     else {       throw new IllegalArgumentException(           \"Model must be an instance of CheckBoxListModel!\");     }   }   else {     super.setModel(model);   } }<\/code><\/pre>\n<p>  The author of this code messed it up a bit: they first check that <i>model<\/i> is not <i>CheckBoxListModel<\/i> but then explicitly cast the object to that very type anyway. This will cause the <i>setModel<\/i> method to throw a <i>ClassCastException<\/i> once it gets to this spot.<\/p>\n<p>  The <i>CheckBoxList.java<\/i> 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. <\/p>\n<p>  If someone relies on this method and reads the Javadoc comment, they will expect an <i>IllegalArgumentException<\/i> rather than a <i>ClassCastException<\/i>. I don&#8217;t think anyone would consciously handle this exception, but you never know.<\/p>\n<p>  Based on the documentation, this is what I think the code should look like:<\/p>\n<pre><code class=\"java\">public void setModel(ListModel model) {   if (!(model instanceof CheckBoxListModel)) {      throw new IllegalArgumentException(         \"Model must be an instance of CheckBoxListModel!\");     }   else {     super.setModel(model);   } }<\/code><\/pre>\n<p>  <\/p>\n<h2>Warning 9<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6060\/\">V6060<\/a> The &#8216;player&#8217; reference was utilized before it was verified against null. VigeanIntuition.java(79), VigeanIntuition.java(78)<\/p>\n<pre><code class=\"java\">@Override public boolean apply(Game game, Ability source) {     MageObject sourceObject = game.getObject(source.getSourceId());     Player player = game.getPlayer(source.getControllerId());     Library library = player.getLibrary();                           \/\/ &lt;=     if (player != null &amp;&amp; sourceObject != null &amp;&amp; library != null) { \/\/ &lt;=         ....     } }<\/code><\/pre>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6060\/\">V6060<\/a> warnings indicate situations where an object is used before being checked for <i>null<\/i>. You can often see such warnings mentioned in articles about checks of open-source projects: they usually result from sloppy refactoring or changing methods&#8217; contracts. If you look at the declaration of the <i>getPlayer()<\/i> method, the problem becomes clear:<\/p>\n<pre><code class=\"java\">\/\/ Result must be checked for null. \/\/ Possible errors search pattern: (\\S*) = game.getPlayer.+\\n(?!.+\\1 != null) Player getPlayer(UUID playerId);<\/code><\/pre>\n<p>  <\/p>\n<h2>Warning 10<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6072\/\">V6072<\/a> Two similar code fragments were found. Perhaps, this is a typo and &#8216;playerB&#8217; variable should be used instead of &#8216;playerA&#8217;. SubTypeChangingEffectsTest.java(162), SubTypeChangingEffectsTest.java(158), SubTypeChangingEffectsTest.java(156), SubTypeChangingEffectsTest.java(160)<\/p>\n<pre><code class=\"java\">@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\");   \/\/ &lt;=      addCard(Zone.HAND, playerB, \"Silvercoat Lion\");     addCard(Zone.BATTLEFIELD, playerB, \"Silvercoat Lion\");     addCard(Zone.GRAVEYARD, playerA, \"Silvercoat Lion\");   \/\/ &lt;=      ....      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));         }     } }<\/code><\/pre>\n<p>  The fact that this defect was found in tests may force you to underestimate its significance (&#171;It&#8217;s not a big deal with tests.&#187;) 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&#8217;s tests and testers that fingers are pointed at. Flawed tests are worthless. Then why have them? Why waste resources on them?<\/p>\n<p>  The <i>testArcaneAdaptationGiveType()<\/i> method tests the &#171;Arcane Adaptation&#187; card. Each player is dealt cards in several playing zones. Thanks to copy-paste, <i>playerA<\/i> will get two &#171;Silvercoat Lion&#187; cards in the &#171;Graveyard&#187; zone, while <i>playerB<\/i> will have none. This is followed by some magic and then the actual testing happens in the end.<\/p>\n<p>  When it comes to testing <i>playerB<\/i>&#8216;s &#171;graveyard&#187; in the current deal, the test never enters the loop since the &#171;graveyard&#187; has been empty from the start. I found this out using the good old <i>System.out.println()<\/i> method when running the test.<\/p>\n<p>  The fixed copy-paste:<\/p>\n<pre><code class=\"java\">.... addCard(Zone.HAND, playerA, \"Silvercoat Lion\"); addCard(Zone.BATTLEFIELD, playerA, \"Silvercoat Lion\"); addCard(Zone.GRAVEYARD, playerA, \"Silvercoat Lion\");   \/\/ &lt;=  addCard(Zone.HAND, playerB, \"Silvercoat Lion\"); addCard(Zone.BATTLEFIELD, playerB, \"Silvercoat Lion\"); addCard(Zone.GRAVEYARD, playerB, \"Silvercoat Lion\");   \/\/ &lt;= ....<\/code><\/pre>\n<p>  Once I&#8217;ve fixed the code, the test was able to check the creatures in <i>playerB<\/i>&#8216;s &#171;graveyard&#187;. Viva <i>System.out.println()<\/i>! <\/p>\n<p>  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. <\/p>\n<p>  A few more copy-paste related defects of this type:<\/p>\n<ul>\n<li>V6072 Two similar code fragments were found. Perhaps, this is a typo and &#8216;playerB&#8217; variable should be used instead of &#8216;playerA&#8217;. PaintersServantTest.java(33), PaintersServantTest.java(29), PaintersServantTest.java(27), PaintersServantTest.java(31)<\/li>\n<li>V6072 Two similar code fragments were found. Perhaps, this is a typo and &#8216;playerB&#8217; variable should be used instead of &#8216;playerA&#8217;. SubTypeChangingEffectsTest.java(32), SubTypeChangingEffectsTest.java(28), SubTypeChangingEffectsTest.java(26), SubTypeChangingEffectsTest.java(30)<\/li>\n<\/ul>\n<p>  <\/p>\n<h2>Warning 11<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6086\/\">V6086<\/a> Suspicious code formatting. &#8216;else&#8217; keyword is probably missing. DeckImporter.java(23)<\/p>\n<pre><code class=\"java\">public static DeckImporter getDeckImporter(String file) {   if (file == null) {     return null;   } if (file.toLowerCase(Locale.ENGLISH).endsWith(\"dec\")) {   \/\/ &lt;=     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;   } }<\/code><\/pre>\n<p>  The <a href=\"https:\/\/www.viva64.com\/en\/w\/v6086\/\">V6086<\/a> diagnostic rule detects incorrectly formatted <i>if-else-if<\/i> constructs, with the <i>else<\/i> keyword missing.<\/p>\n<p>  This is exactly what the snippet above demonstrates. In this case, the inaccurate formatting won&#8217;t do any harm thanks to the <i>return null <\/i>statement, but it&#8217;s cool to find issues like that since you can never be sure. <\/p>\n<p>  Here&#8217;s an example of how a missing <i>else<\/i> leads to unexpected behavior:<\/p>\n<pre><code class=\"java\">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; }<\/code><\/pre>\n<p>  Now, if <i>obj == null<\/i>, the object will be assigned some value and, because of the missing <i>else<\/i>, forced to go down the entire <i>if-else-if<\/i> line for checking, while the original idea was to have the object returned right off. <\/p>\n<h2>Conclusion<\/h2>\n<p>  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&#8217;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?<\/p>\n<p>  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&#8217;s diagnostic rules or from overcomplicated code, which the analyzer has a difficult time figuring out. Please be understanding about it and don&#8217;t hesitate to <a href=\"https:\/\/www.viva64.com\/en\/about-feedback\/\">report<\/a> any false positives you encounter, and while they are waiting to be fixed, you can use one of the warning suppression <a href=\"https:\/\/www.viva64.com\/en\/m\/0044\/\">mechanisms<\/a>. <\/p>\n<p>  Feel free to <a href=\"https:\/\/www.viva64.com\/en\/pvs-studio-download\/\">download<\/a> the analyzer and give it a try.<\/div>\n<\/div>\n<\/div>\n<p><!----><!----><\/div>\n<p><!----><!----><br \/> \u0441\u0441\u044b\u043b\u043a\u0430 \u043d\u0430 \u043e\u0440\u0438\u0433\u0438\u043d\u0430\u043b \u0441\u0442\u0430\u0442\u044c\u0438 <a href=\"https:\/\/habr.com\/ru\/articles\/516974\/\"> https:\/\/habr.com\/ru\/articles\/516974\/<\/a><\/p>\n","protected":false},"excerpt":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-1\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<div style=\"text-align:center;\"><img decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/getpro\/habr\/post_images\/ab5\/41b\/2b4\/ab541b2b4f379f0af908f736888b87f2.png\" alt=\"image1.png\" data-src=\"https:\/\/habrastorage.org\/getpro\/habr\/post_images\/ab5\/41b\/2b4\/ab541b2b4f379f0af908f736888b87f2.png\"\/><\/div>\n<p>  XMage is a client-server application for playing Magic: The Gathering (MTG). XMage&#8217;s development was started in early 2010. Since then, it has seen 182 releases, attracted an army of contributors, and it&#8217;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.  <\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[],"tags":[],"class_list":["post-407715","post","type-post","status-publish","format-standard","hentry"],"_links":{"self":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/407715","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=407715"}],"version-history":[{"count":0,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/407715\/revisions"}],"wp:attachment":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=407715"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=407715"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=407715"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}