Clever Geek Handbook

检查XMage代码以及为何无法使用Dragon's Maze系列的特殊稀有卡

image1.png


XMage是用于播放Magic：The Gathering（MTG）的客户端/服务器应用程序。XMage于2010年初开始发展。在此期间，发布了182个发行版，一大批贡献者聚集在一起，并且该项目仍在积极开发中。也是参与其发展的绝佳机会！因此，今天来自PVS-Studio的独角兽将检查XMage代码库，谁知道呢，它可能会与战斗中的某个人发生冲突。



简要介绍该项目



XMage已经积极开发了10年。其目标是制作原始《魔术师：聚会》纸牌游戏的免费开放源在线版本。



应用特点：



  • 拥有20年来MTG发行的约19,000张独特卡；
  • 自动控制和应用所有现有游戏规则；
  • ;
  • (AI);
  • (Standard, Modern, Vintage, Commander );
  • , .




偶然发现了代尔夫特理工大学2018年学生的工作（软件架构硕士学位）。这包括以下事实：这些家伙积极参与了开源项目，因为开源项目必须相当复杂并且要积极开发。在八周的时间内，学生学习了课程和开源项目，以了解和描述所选软件的体系结构。



就是这样了。在这项工作中，这些家伙分析了XMage项目，他们的工作之一就是使用SonarQube获得各种度量（代码行数，圈复杂度，代码重复，代码气味，错误，漏洞等）。



在2018年SonarQube扫描时，每1,000,000行代码显示700个缺陷（漏洞，漏洞），这一事实吸引了我的注意力。



在深入研究贡献者的历史之后，我发现从收到警告的报告中，他们提出了撤消请求，以修复“障碍”或“关键”类别中的大约30个缺陷。其余的警告信息尚不清楚，但我希望不要错过它们。



从那时到现在已经两年了，代码库已经增长了大约25万行代码-这是了解事情进展的一个很好的理由。



关于分析



为了进行分析，我选择了XMage版本-1.4.44V0。



我对这个项目很幸运。事实证明，使用Maven构建XMage非常简单（如文档中所写）： 



mvn clean install -DskipTests


我什么都不需要了。凉？



将PVS-Studio插件集成到Maven中也没有问题：一切都与文档中的一样。



分析之后，收到了911警告，其中674是关于1和2置信度的警告。出于本文的目的，我没有考虑3级警告，因为通常会出现误报率很高的情况。我想提请您注意以下事实：在实际战斗中使用静态分析器时，您不能忽略此类警告，因为它们也可能表示代码中的重大缺陷。



另外，我之所以没有考虑某些规则的警告，是因为与我相比，熟悉该项目的人对它们的考虑更好：



  • V6022, /. 336 .
  • V6014, , . 73 .
  • V6021, , . 36 .
  • V6048, , . 17 .


另外，一些诊断规则产生了大约20个相同类型的明显假阳性。记录在待办事项中！



结果，如果我们减去所有内容，那么大约有190个正值可供我考虑。



在检查触发器时，发现了许多相同类型的较小缺陷，这些缺陷与调试或无意义的检查或操作有关。而且，很多积极的事情与乞求重构的一段非常奇怪的代码有关。



结果，对于本文，我确定了11条诊断规则，并分析了最有趣的触发器之一。



让我们看看发生了什么。



警告N1



V6003检测到使用'if（card！= Null）{...} else if（card！= Null）{...}'模式。存在逻辑错误的可能性。TorrentialGearhulk.java（90），TorentialGearhulk.java（102） 



@Override
public boolean apply(Game game, Ability source) {
  ....
  Card card = game.getCard(....);
  if (card != null) {
      ....
  } else if (card != null) {
      ....
  }
  ....
}


这里的一切都很简单：if-else-if构造中的第二个条件语句if（card！= Null）的主体将永远不会执行，因为程序要么不会到达这一点，要么card！= Null始终为false。



警告N2



V6004'then '语句等效于'else'语句。AsThoughEffectImpl.java（35），AsThoughEffectImpl.java（37） 



@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);
  }
}


在检查开放源代码项目的实践中经常发生的常见错误。复制粘贴？还是我错过了什么？我将假设您仍然需要在else分支中返回false。



PS如果有任何内容，则没有递归调用适用（....），因为它们是不同的方法。



类似触发：



  • V6004'then'语句等效于'else'语句。GuiDisplayUtil.java（194），GuiDisplayUtil.java（198）


警告N3



V6007表达式'filter.getMessage（）。ToLowerCase（Locale.ENGLISH）.startsWith（“ Each”）'始终为false。SetPowerToughnessAllEffect.java（107） 



@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 诊断规则触发器在每个要检查的项目中都很流行。 XMage也不例外（79件）。原则上，规则的执行取决于情况，但许多情况取决于调试，然后是再保险，然后是其他情况。通常，对于代码编写者来说，这种肯定比对我来说更好。



但是，此操作绝对是错误的。根据行的开头filter.getMessage（）到sb添加文本“有...”或“有...”。但是错误是开发人员检查了字符串是否以大写字母开头，然后才将该字符串转换为小写字母。哎呀。结果，添加的行将始终为“ have ...”。缺陷的结果并不关键，但也不令人愉悦：在某处会出现乱码的文本。



我发现最有趣的优点：



  • V6007表达式't.startsWith（“-”）'始终为false。BoostSourceEffect.java（103）
  • V6007表达式“ setNames.isEmpty（）”始终为false。下载PicturesService.java（300）
  • V6007表达式“ existingBucketName == null”始终为false。S3Uploader.java（23）
  • V6007表达式'！LastRule.endsWith（“。”）'始终为true。Effects.java（76）
  • V6007表达式'subtypesToIgnore :: contains'始终为false。VerifyCardDataTest.java（893）
  • V6007表达式“ notStartedTables == 1”始终为false。MageServerImpl.java（1330）


警告N4



V6008对“ savedSpecialRares”的空引用。DragonsMaze.java（230） 



public final class DragonsMaze extends ExpansionSet {
  ....
  private List<CardInfo> savedSpecialRares = new ArrayList<>();
  ....
  @Override
  public List<CardInfo> 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);
  }
}


当执行到达集合的第一个填充时 ，解析器抱怨取消对空引用saveSpecialRares的引用。



首先想到的只是将saveSpecialRares == null与savedSpecialRares！= Null混淆。但是在这种情况下，从方法返回集合时，NPE可能会在ArrayList的构造函数中发生，因为saveSpecialRares == null仍然是可能的。用想到的第一个解决方案修复代码不是一个好的选择。在对代码有一点了解之后，我发现s avedSpecialRares在声明时立即由一个空集合定义，而不在其他任何地方重新分配。这告诉我们savedSpecialRares将永远不会为null，并且不会发生分析器警告的对null引用的取消引用，因为它永远不会到达集合。结果，该方法将始终返回空集合。



PS要修复此问题，您需要将savedSpecialRares == null替换为savedSpecialRares.isEmpty（）。



PPS las，在使用XMage时，您将无法获得Dragon's Maze系列的特殊稀有卡。



取消引用空引用的另一种情况：



  • V6008取消对“ match”的引用。TableController.java（973）


警告N5



V6012 ' ？： '运算符，无论其条件表达式如何，始终返回一个相同的值'table.getCreateTime（）'。TableManager.java（418），TableManager.java（418） 



private void checkTableHealthState() {
  ....
  logger.debug(.... + formatter.format(table.getStartTime() == null
                                        ? table.getCreateTime()
                                        : table.getCreateTime()) + ....);
  ....
}


这里的三元运算符？：不管条件table.getStartTime（）== null，返回相同的值。我相信代码完成对开发人员来说是一个残酷的笑话。校正选项： 



private void checkTableHealthState() {
  ....
  logger.debug(.... + formatter.format(table.getStartTime() == null
                                        ? table.getCreateTime()
                                        : table.getStartTime()) + ....);
  ....
}


警告N6



V6026该值已经分配给'this.loseOther'变量。成为CreatureTypeTargetEffect.java（54） 



public
BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {
  super(effect);
  this.subtypes.addAll(effect.subtypes);
  this.loseOther = effect.loseOther;
  this.loseOther = effect.loseOther;
}


分配字符串重复。似乎开发人员对使用热键有些不满意，并且没有注意到它。但是，鉴于效果具有大量字段，因此应该重点关注片段。



警告N7



V6036使用未初始化的'selectUser'可选值。Session.java（227） 



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(); // <=
  ....
}


从分析器的警告中，我们可以得出结论，selectUser.get（）可能会引发NoSuchElementException。



让我们仔细看看这里发生了什么。



如果您认为该用户已存在的注释，则不会抛出异常： 



....
if (!selectUser.isPresent()) {  // user already exists
  ....
}
User user = selectUser.get()
....


在这种情况下，程序执行将不会进入条件语句的主体。一切都会好起来的。但是随后出现了一个问题：如果没有执行条件运算符，为什么我们需要一个带有一些复杂逻辑的条件运算符？



但是，如果评论什么都没有，该怎么办？ 



....
if (!selectUser.isPresent()) {  // user already exists
    selectUser = UserManager.instance.getUserByName(userName);
    if (selectUser.isPresent()) {
      ....
    }
}
User user = selectUser.get(); // <=
....


然后执行进入条件语句的主体，并通过getUserByName（）重新获取用户。再次检查用户的有效性，这表明selectUser可能未初始化。在这种情况下没有其他分支，这将进一步在所讨论的代码行上导致NoSuchElementException。



警告N8



V6042已检查表达式是否与类型'A'兼容，但将其强制转换为类型'B'。 CheckBoxList.java（586） 



/**
 * 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);
  }
}


代码的作者对此有些困惑：首先，他确保模型不是CheckBoxListModel，然后结果将对象显式转换为该类型。因此，setModel方法到达那里时将立即引发ClassCastException。CheckBoxList.java



文件是2年前添加的，此错误一直存在于代码中。显然，没有针对不正确参数的测试，没有针对不适当类型的对象真正使用此方法，因此它存在。 如果突然有人挂接到此方法并读取Javadoc，他们将期望收到IllegalArgumentException而不是ClassCastException



... 我认为没有人会故意遇到这种例外，但是谁知道呢。



根据文档，代码很可能看起来像这样： 



public void setModel(ListModel model) {
  if (!(model instanceof CheckBoxListModel)) {
     throw new IllegalArgumentException(
        "Model must be an instance of CheckBoxListModel!");  
  }
  else {
    super.setModel(model);
  }
}


警告N9



V6060在验证是否为null之前，已使用“玩家”参考。VigeanIntuition.java（79），VigeanIntuition.java（78​​） 



@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警告开发人员在检查对象是否为null之前，正在访问该对象。经常在有关检查开源项目的文章中找到此规则的触发因素：通常原因是重构失败或更改方法合同。如果您注意getPlayer（）方法的声明，那么一切将立即生效： 



// Result must be checked for null.
// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)
Player getPlayer(UUID playerId);


警告N10



V6072找到两个相似的代码片段。也许这是一个错字，应该使用“ playerB”变量而不是“ playerA”。 SubTypeChangingEffectsTest.java（162），SubTypeChangingEffectsTest.java（158），SubTypeChangingEffectsTest.java（156），SubTypeChangingEffectsTest.java（160） 



@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));
        }
    }
}


看到此错误已在测试中后，您可以立即对发现的缺陷进行贬值，以为：“好吧，这些就是测试。”如果是这样，那么我不同意你的看法。毕竟，测试在开发中起着相当重要的作用（尽管不像编程那样引人注目），并且当发布中出现缺陷时，它们立即开始将手指指向测试/测试人员。因此，有缺陷的测试是站不住脚的。那么为什么需要这样的测试？为什么要浪费资源呢？testArcaneAdaptationGiveType（）



方法测试“奥术适应”卡。每个玩家都被发牌到特定的游戏区域。多亏了复制粘贴，玩家A在“公墓”游戏区域获得了两张相同的“ Silvercoat Lion”卡，而玩家B所以什么都没有。此外，还有某种魔术和测试本身。



当测试到达当前回合中玩家B的“墓地”时，测试执行就永远不会进入循环，因为“墓地”中什么也没有。在开始测试时，我用旧的System.out.println（）发现了这一点。



更正的复制粘贴： 



....
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");   // <=
....


调整代码后，当我运行测试时，检查playerB墓地中的生物开始起作用。Ave，System.out.println（）！



校正前后，测试均为绿色，这很幸运。但是，如果有任何修改改变了程序执行的逻辑，那么这样的测试将对您不利，即使有错误也通知您成功完成。



在其他地方相同的复制粘贴：



  • V6072找到两个相似的代码片段。也许这是一个错字，应该使用“ playerB”变量而不是“ playerA”。PaintersServantTest.java（33），PaintersServantTest.java（29），PaintersServantTest.java（27），PaintersServantTest.java（31）
  • V6072找到两个相似的代码片段。也许这是一个错字，应该使用“ playerB”变量而不是“ playerA”。SubTypeChangingEffectsTest.java（32），SubTypeChangingEffectsTest.java（28），SubTypeChangingEffectsTest.java（26），SubTypeChangingEffectsTest.java（30）


警告N11



V6086可疑代码格式。'else'关键字可能丢失。DeckImporter.java（23） 



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;
  }
}


诊断规则V6086诊断不正确的if-else-if格式，这意味着省略else。



此代码段演示了这一点。在这种情况下，由于返回的是null表达式，格式上的不准确性不会导致任何问题，但是找到这种情况很酷，因为您不必这样做。



让我们考虑一种情况，其中忽略else可能导致意外行为： 



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;
}


现在，在obj == null的情况下，将为有问题的对象分配一些值，而缺少的其他对象将导致新分配的对象开始沿if-else-if链进行检查，而该对象应立即从以下位置返回方法。



结论



检查XMage是另一篇文章，揭示了现代静态分析仪的功能。在现代开发中，随着软件复杂性的增加，对它们的需求只会增长。不管您有多少版本，测试和用户反馈：一个错误总是会发现一个漏洞，进入您的代码库。那么，为什么不增加防御的另一个障碍呢？



如您所知，分析仪容易出现误报（包括PVS-Studio Java）。这既可能是明显的缺陷，也可能是代码过于混乱的结果（可惜分析器没有弄清楚它）。您需要以谅解的态度对待他们，并立即毫不犹豫地退订，但是当误报正在等待其纠正时，您可以使用其中一种方法禁止警告。



最后，我建议您亲自从我们的网站下载“触摸”分析仪。





如果您想与讲英语的读者分享本文，请使用翻译链接：Maxim Stefanov。检查XMage的代码，以及为什么您无法获得Dragon's Maze系列的特殊稀有卡。


More articles:


All Articles