XMageコードを確認し、Dragon'sMazeコレクションの特別なレアカードが利用できない理由

XMageは、Magic：The Gathering（MTG）をプレイするためのクライアント/サーバーアプリケーションです。XMageは2010年の初めに進化し始めました。この間、182のリリースがリリースされ、貢献者の軍隊全体が集まり、プロジェクトは現在も活発に開発されています。その開発にも参加する絶好の機会です！したがって、今日、PVS-StudioのユニコーンはXMageコードベースをチェックします。誰が知っているか、それは戦闘中の誰かと衝突する可能性があります。

プロジェクトについて簡単に

XMageは10年前から活発に開発されています。その目標は、オリジナルのMagic：TheGatheringカードゲームの無料のオープンソースのオンラインバージョンを作成することです。



アプリケーションの機能：

• MTGの20年の歴史の中で発行された約19,000枚のユニークなカードへのアクセス。
• ゲームの既存のすべてのルールの自動制御と適用。
• ;
• (AI);
• (Standard, Modern, Vintage, Commander );
• , .

Delft University of Technology 2018（ソフトウェアアーキテクチャの修士号）の学生 の仕事に出くわしました。それは、彼らがオープンソースプロジェクトに積極的に参加したという事実から成り立っていました。それは非常に複雑で積極的に開発されなければなりませんでした。学生は8週間にわたって、選択したソフトウェアのアーキテクチャを理解して説明するために、コースとオープンソースプロジェクトを研究しました。 以上です。この作業では、XMageプロジェクトを分析し、SonarQubeを使用してさまざまなメトリック（コードの行数、サイクロマティックな複雑さ、コードの重複、コードの臭い、エラー、脆弱性など）を取得することを行いました。







私の注意は、2018年の時点でSonarQubeスキャンが1,000,000行のコードあたり700の欠陥（バグ、脆弱性）を示したという事実に惹かれました。



寄稿者の歴史を掘り下げたところ、受け取った警告付きのレポートから、「ブロッカー」または「クリティカル」カテゴリから約30個の欠陥を修正するようにプルリクエストを行ったことがわかりました。残りの警告については不明ですが、見逃されていないことを願っています。



それから2年が経ち、コードベースは約250,000行のコードで成長しました。これは、状況がどうなっているのかを確認するのに十分な理由です。

分析について

分析のために、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件発生しました。todoに収録！



その結果、すべてを差し引くと、約190のポジティブが検討のために私に届きました。



トリガーを確認しているときに、同じタイプの多くの小さな欠陥が特定されました。これらは、デバッグまたは無意味なチェックまたは操作に関連していました。また、多くのポジティブな点は、リファクタリングを懇願する非常に奇妙なコードに関連していました。



その結果、この記事では、11の診断ルールを特定し、最も興味深いトリガーの1つを分析しました。



何が起こったのか見てみましょう。

警告N1

V6003'if（card！= Null）{...} else if（card！= Null）{...} 'パターンの使用が検出されました。論理エラーが存在する可能性があります。TorrentialGearhulk.java（90）、TorrentialGearhulk.java（102）

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

ここではすべてが単純です。プログラムがこのポイントに到達しないか、card！= Nullが常にfalseになるため、if-else-if構文の2番目の条件ステートメントif（card！= Null）の本体は実行されません。

警告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「has ...」または「have ...」というテキストが追加されます。しかし、間違いは、開発者が文字列が大文字で始まることを確認し、その前にこの同じ文字列を小文字に変換したことです。おっと。その結果、追加された行は常に「have ...」になります。欠陥の結果は重大ではありませんが、不快でもあります。文字通りに構成されていないテキストがどこかに表示されます。



私が最も興味深いと思ったトリガー：

• V6007式 't.startsWith（ "-"）'は常にfalseです。BoostSourceEffect.java（103）
• V6007式 'setNames.isEmpty（）'は常にfalseです。DownloadPicturesService.java（300）
• V6007式 'existingBucketName == null'は常にfalseです。S3Uploader.java（23）
• V6007式 '！LastRule.endsWith（ "。"）'は常にtrueです。Effects.java（76）
• V6007式 'subtypesToIgnore :: contains'は常にfalseです。VerificationCardDataTest.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);
  }
}

パーサーは、実行がコレクションの最初の入力に達したときに、null参照savedSpecialRaresの参照を解除することについて文句を言います。



最初に頭に浮かぶのは、savedSpecialRares == nullとsavedSpecialRares！= Nullを単純に混同することです。ただし、このような場合、savedSpecialRares == nullが引き続き可能であるため、コレクションがメソッドから返されるときにArrayListのコンストラクターでNPEが発生する可能性があります。頭に浮かぶ最初の解決策でコードを修正することは良い選択肢ではありません。コードを少し理解した後、s avedSpecialRaresは、宣言されるとすぐに空のコレクションによって定義され、他の場所に再割り当てされないことがわかりました。これは私たちにそれを伝えますsavedSpecialRaresがnullになることはなく、コレクションに到達することはないため、アナライザーが警告するnull参照の逆参照は発生しません。その結果、メソッドは常に空のコレクションを返します。



PSこれを修正するには、savedSpecialRares == nullをsavedSpecialRares.isEmpty（）に置き換える必要があります。



PPS残念ながら、XMageをプレイしている間は、Dragon'sMazeコレクションの特別なレアカードを入手することはできません。



null参照を逆参照する別のケース：

• V6008「一致」のヌル逆参照。TableController.java（973）

警告N5

V6012 '？：'演算子は、その条件式に関係なく、常に1つの同じ値 'table.getCreateTime（）'を返します。TableManager.java（418）、TableManager.java（418）

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

ここで、3値演算子？：条件table.getStartTime（）== nullに関係なく同じ値を返します。コードの完成は開発者に残酷な冗談を言ったと思います。修正オプション：

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

警告N6

V6026この値は、「this.loseOther」変数にすでに割り当てられています。GetsCreatureTypeTargetEffect.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を読み取ると、ClassCastExceptionではなくIllegalArgumentExceptionが発生することが予想されます。



..。意図的にこの例外に遭遇する人はいないと思いますが、誰が知っていますか。



ドキュメントを考えると、コードはおそらく次のようになります。

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

V60722つの類似したコードフラグメントが見つかりました。おそらく、これはタイプミスであり、「playerA」の代わりに「playerB」変数を使用する必要があります。 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（）



メソッドは、「ArcaneAdaptation」カードをテストします。各プレイヤーには、特定のプレイエリアにカードが配られます。そして、コピー＆ペーストのおかげで、playerAは「Cemetery」プレイエリアで2枚の同一の「SilvercoatLion」カードを手に入れました。だから何も得られなかった。次に、いくつかの魔法とそれ自体をテストします。現在の図面のplayerBの



「墓地」にテストが来ると、「墓地」には何もなかったため、テストの実行がループに入ることはありません。これは、テストを開始したときに古き良き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の墓地にいるクリーチャーのチェックが機能し始めました。アベニュー、System.out.println（）！



修正前と修正後の両方でテストが緑色になり、幸運です。しかし、プログラム実行のロジックを変更する編集の場合、そのようなテストはあなたに不利益をもたらし、エラーがあったとしても正常に完了したことを通知します。



同じコピー-他の場所に貼り付けます：

• V60722つの類似したコードフラグメントが見つかりました。おそらく、これはタイプミスであり、「playerA」の代わりに「playerB」変数を使用する必要があります。PaintersServantTest.java（33）、PaintersServantTest.java（29）、PaintersServantTest.java（27）、PaintersServantTest.java（31）
• V60722つの類似したコードフラグメントが見つかりました。おそらく、これはタイプミスであり、「playerA」の代わりに「playerB」変数を使用する必要があります。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の場合、問題のオブジェクトには何らかの値が割り当てられ、elseが欠落していると、新しく割り当てられたオブジェクトがif-else-ifチェーンに沿ってチェックされ始めますが、オブジェクトはすぐに方法。

結論

XMageのチェックは、最新の静的アナライザーの機能を明らかにする別の記事です。現代の開発では、ソフトウェアの複雑さが増すにつれて、それらの必要性は高まるだけです。また、リリース、テスト、ユーザーフィードバックの数に関係なく、バグは常にコードベースに侵入するための抜け穴を見つけます。では、防御に別の障壁を追加してみませんか？



ご存知のように、アナライザーは誤検知を起こしやすい傾向があります（PVS-Studio Javaを含む）。これは、明らかな欠陥と混乱しすぎるコードの両方の結果である可能性があります（ああ、アナライザーはそれを理解しませんでした）。あなたはそれらを理解して扱い、ためらうことなくすぐに退会する必要がありますが、誤検知がそれらの修正を待っている間、あなたはいずれかの方法を使用することができます警告を抑制します。



結論として、私たちのウェブサイトからアナライザーをダウンロードして、個人的にアナライザーに「触れる」ことをお勧めします。

この記事を英語を話す聴衆と共有したい場合は、翻訳リンクを使用してください：マキシムステファノフ。XMageのコードを確認し、ドラゴンの迷路コレクションの特別なレアカードを入手できない理由。