
XMage adalah aplikasi klien / server untuk bermain Magic: The Gathering (MTG). XMage mulai berkembang kembali pada awal 2010. Selama waktu ini, 182 rilis telah dirilis, seluruh pasukan kontributor berkumpul, dan proyek masih aktif berkembang. Kesempatan bagus untuk berpartisipasi dalam perkembangannya juga! Oleh karena itu, hari ini unicorn dari PVS-Studio akan memeriksa basis kode XMage, dan siapa tahu, mungkin bentrok dengan seseorang dalam pertempuran.
Secara singkat tentang proyek
XMage telah aktif berkembang selama 10 tahun sekarang. Tujuannya adalah membuat game kartu Magic: the Gathering versi online gratis dan open source .
Fitur aplikasi:
- akses ke ~ 19.000 kartu unik yang diterbitkan selama 20 tahun sejarah MTG;
- kontrol otomatis dan penerapan semua aturan permainan yang ada;
- ;
- (AI);
- (Standard, Modern, Vintage, Commander );
- , .
Tersandung pada pekerjaan siswa dari Delft University of Technology 2018 (program Magister Arsitektur Perangkat Lunak ). Terdiri dari fakta bahwa orang-orang tersebut mengambil bagian aktif dalam proyek sumber terbuka, yang harus cukup kompleks dan secara aktif berkembang. Selama periode delapan minggu, siswa mempelajari kursus dan proyek sumber terbuka untuk memahami dan mendeskripsikan arsitektur perangkat lunak yang dipilih.
Jadi begitulah. Dalam pekerjaan ini, orang-orang menganalisis proyek XMage, dan salah satu aspek pekerjaan mereka adalah memperoleh berbagai metrik menggunakan SonarQube (jumlah baris kode, kompleksitas siklomatik, duplikasi kode, bau kode, kesalahan, kerentanan, dll.).
Perhatian saya tertuju pada fakta bahwa pada saat pemindaian SonarQube 2018 menunjukkan 700 cacat (bug, kerentanan) per 1.000.000 baris kode.
Setelah menggali sejarah orang-orang yang berkontribusi, saya menemukan bahwa dari laporan yang diterima dengan peringatan mereka membuat permintaan-tarik untuk memperbaiki sekitar 30 cacat dari kategori "Pemblokir" atau "Kritis". Bagaimana dengan peringatan lainnya yang tidak diketahui, tapi saya harap mereka tidak diabaikan.
Sudah 2 tahun sejak itu dan basis kode telah berkembang sekitar 250.000 baris kode - alasan yang bagus untuk melihat bagaimana perkembangannya.
Tentang analisis
Untuk analisis, saya mengambil rilis XMage - 1.4.44V0 .
Saya sangat beruntung dengan proyek ini. Membangun XMage menggunakan Maven ternyata sangat sederhana (seperti yang tertulis di dokumentasi):
mvn clean install -DskipTests
Tidak ada lagi yang diminta dari saya. Keren?
Tidak ada masalah dengan integrasi plugin PVS-Studio ke Maven: semuanya seperti dalam dokumentasi .
Setelah analisis, 911 peringatan diterima, 674 di antaranya adalah peringatan dengan tingkat kepercayaan 1 dan 2. Untuk keperluan artikel ini, saya belum mempertimbangkan peringatan level 3, karena biasanya ada persentase positif palsu yang tinggi. Saya ingin menarik perhatian Anda pada fakta bahwa ketika menggunakan penganalisis statis dalam pertempuran nyata, Anda tidak dapat mengabaikan peringatan seperti itu, karena mereka juga dapat menunjukkan kerusakan yang signifikan pada kode.
Selain itu, saya tidak mempertimbangkan peringatan dari beberapa aturan karena lebih baik dipertimbangkan oleh mereka yang akrab dengan proyek daripada saya:
- V6022, /. 336 .
- V6014, , . 73 .
- V6021, , . 36 .
- V6048, , . 17 .
Selain itu, beberapa aturan diagnostik menghasilkan sekitar 20 positif palsu yang jelas dari jenis yang sama. Direkam dalam agenda!
Akibatnya, jika kita mengurangi semuanya, maka sekitar 190 hal positif harus saya pertimbangkan.
Saat meninjau pemicu, banyak cacat kecil dari jenis yang sama diidentifikasi, yang terkait dengan debugging atau pemeriksaan atau operasi yang tidak berarti. Selain itu, banyak hal positif dikaitkan dengan potongan kode yang sangat aneh yang meminta untuk refactoring.
Akibatnya, untuk artikel ini saya telah mengidentifikasi 11 aturan diagnostik dan menganalisis salah satu pemicu paling menarik.
Mari kita lihat apa yang terjadi.
Peringatan N1
V6003 Penggunaan pola 'if (card! = Null) {...} else if (card! = Null) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. 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) {
....
}
....
}
Semuanya sederhana di sini: isi pernyataan kondisional kedua if (card! = Null) dalam konstruksi if-else-if tidak akan pernah dijalankan, karena program tidak akan mencapai titik ini, atau card! = Null akan selalu salah .
Peringatan N2
V6004 Pernyataan 'then' setara dengan pernyataan '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);
}
}
Kesalahan dangkal yang sering terjadi dalam praktik saya memeriksa proyek open source. Salin-tempel? Atau apakah saya melewatkan sesuatu? Saya akan berasumsi bahwa Anda masih perlu mengembalikan false di cabang lain . PS Jika ada, tidak ada panggilan rekursif berlaku (....) , karena ini adalah metode yang berbeda. Pemicuan serupa:
- V6004 Pernyataan 'then' setara dengan pernyataan 'else'. GuiDisplayUtil.java (194), GuiDisplayUtil.java (198)
Peringatan N3
V6007 Expression 'filter.getMessage (). ToLowerCase (Locale.ENGLISH) .startsWith ("Each")' selalu salah. 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();
}
Pemicu aturan diagnostik V6007 cukup populer untuk setiap proyek yang diperiksa. XMage tidak terkecuali (79 buah). Penerapan aturan, pada prinsipnya, semuanya ada pada kasusnya, tetapi banyak kasus jatuh pada debug, lalu pada reasuransi, lalu pada hal lain. Secara umum, hal-hal positif seperti itu lebih baik untuk diperhatikan penulis kode daripada untuk saya.
Pemicuan ini, bagaimanapun, jelas merupakan kesalahan. Bergantung pada awal baris filter.getMessage () ke sbteks "memiliki ..." atau "memiliki ..." ditambahkan. Tetapi kesalahannya adalah bahwa pengembang memeriksa bahwa string dimulai dengan huruf kapital, setelah sebelumnya mengubah string ini menjadi huruf kecil. Ups. Akibatnya, baris yang ditambahkan akan selalu "memiliki ...". Hasil dari cacat tersebut tidak kritis, tetapi juga tidak menyenangkan: teks yang disusun secara buta huruf akan muncul di suatu tempat.
Hal positif yang menurut saya paling menarik:
- V6007 Ekspresi 't.startsWith ("-")' selalu salah. BoostSourceEffect.java (103)
- V6007 Expression 'setNames.isEmpty ()' selalu salah. DownloadPicturesService.java (300)
- V6007 Ekspresi 'existingBucketName == null' selalu salah. S3Uploader.java (23)
- V6007 Expression '! LastRule.endsWith (".")' Selalu benar. Effects.java (76)
- V6007 Expression 'subtypesToIgnore :: contains' selalu salah. VerifyCardDataTest.java (893)
- V6007 Ekspresi 'notStartedTables == 1' selalu salah. MageServerImpl.java (1330)
Peringatan N4
V6008 Dereferensi nol 'saveSpecialRares'. 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);
}
}
Pengurai mengeluh tentang dereferensi referensi null disimpanSpecialRares saat eksekusi mencapai pengisian pertama koleksi.
Hal pertama yang terlintas di benak Anda adalah membingungkan storedSpecialRares == null dengan storedSpecialRares! = Null. Tapi dalam kasus seperti itu, NPE bisa terjadi di konstruktor ArrayList ketika koleksi dikembalikan dari metode, karena saveSpecialRares == null masih memungkinkan. Memperbaiki kode dengan solusi pertama yang terlintas dalam pikiran bukanlah pilihan yang baik. Setelah sedikit memahami kodenya, saya menemukan bahwa s avedSpecialRares langsung ditentukan oleh koleksi kosong ketika dideklarasikan dan tidak ditugaskan di tempat lain. Ini memberitahu kita hal ituSavedSpecialRares tidak akan pernah menjadi null, dan dereferensi referensi null, yang diperingatkan oleh penganalisis, tidak akan pernah terjadi, karena referensi tersebut tidak akan pernah mencapai koleksi. Akibatnya, metode ini akan selalu mengembalikan koleksi kosong.
PS Untuk memperbaikinya, Anda perlu mengganti storedSpecialRares == null dengan storedSpecialRares.isEmpty () .
PPS Sayangnya, saat bermain XMage, kamu tidak akan bisa mendapatkan kartu langka khusus untuk koleksi Labirin Naga .
Kasus lain dereferensi referensi nol:
- V6008 Dereferensi nol 'cocok'. TableController.java (973)
Peringatan N5
V6012 Operator '?:', Terlepas dari ekspresi kondisionalnya, selalu mengembalikan satu dan nilai yang sama 'table.getCreateTime ()'. TableManager.java (418), TableManager.java (418)
private void checkTableHealthState() {
....
logger.debug(.... + formatter.format(table.getStartTime() == null
? table.getCreateTime()
: table.getCreateTime()) + ....);
....
}
Berikut operator ternary : Mengembalikan nilai yang sama terlepas dari kondisi table.getStartTime () == null . Saya percaya bahwa penyelesaian kode telah memainkan lelucon yang kejam bagi pengembang. Opsi koreksi:
private void checkTableHealthState() {
....
logger.debug(.... + formatter.format(table.getStartTime() == null
? table.getCreateTime()
: table.getStartTime()) + ....);
....
}
Peringatan N6
V6026 Nilai ini telah ditetapkan ke variabel 'this.loseOther'. BecomesCreatureTypeTargetEffect.java (54)
public
BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {
super(effect);
this.subtypes.addAll(effect.subtypes);
this.loseOther = effect.loseOther;
this.loseOther = effect.loseOther;
}
Gandakan string tugas. Sepertinya pengembang agak terbawa menggunakan hotkey dan tidak menyadarinya. Namun mengingat efek memiliki sejumlah besar bidang, fragmen layak untuk difokuskan.
Peringatan N7
V6036 Nilai dari opsional 'selectUser' yang tidak diinisialisasi digunakan. 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(); // <=
....
}
Dari peringatan penganalisis, kita dapat menyimpulkan bahwa selectUser.get () mungkin memunculkan NoSuchElementException.
Mari kita lihat lebih dekat apa yang terjadi di sini.
Jika Anda yakin komentar bahwa pengguna tersebut sudah ada, maka tidak ada pengecualian yang akan dilontarkan:
....
if (!selectUser.isPresent()) { // user already exists
....
}
User user = selectUser.get()
....
Dalam hal ini, eksekusi program tidak akan masuk ke dalam isi pernyataan bersyarat. Dan semuanya akan baik-baik saja. Tetapi kemudian muncul pertanyaan: mengapa kita membutuhkan operator bersyarat dengan semacam logika kompleks jika tidak pernah dijalankan?
Tetapi bagaimana jika komentar tersebut tidak ada apa-apanya?
....
if (!selectUser.isPresent()) { // user already exists
selectUser = UserManager.instance.getUserByName(userName);
if (selectUser.isPresent()) {
....
}
}
User user = selectUser.get(); // <=
....
Kemudian eksekusi memasuki isi pernyataan bersyarat dan mendapatkan kembali pengguna melalui getUserByName (). Pengguna diperiksa lagi validitasnya, yang menunjukkan bahwa selectUser mungkin tidak diinisialisasi. Tidak ada cabang lain untuk kasus ini, yang selanjutnya akan mengarah ke NoSuchElementException pada baris kode yang dimaksud.
Peringatan N8
V6042 Ekspresi diperiksa kompatibilitasnya dengan tipe 'A' tapi dilemparkan ke tipe '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);
}
}
Penulis kode agak bingung di sini: pertama dia memastikan bahwa model tersebut bukan CheckBoxListModel , dan kemudian, pada akhirnya, secara eksplisit mentransmisikan objek ke tipe ini. Karena itu, metode setModel akan segera menampilkan ClassCastException saat metode tersebut sampai di sana.
File CheckBoxList.java ditambahkan 2 tahun yang lalu dan bug ini tetap ada di kode sejak saat itu. Rupanya, tidak ada tes untuk parameter yang salah, tidak ada penggunaan nyata dari metode ini dengan objek dari jenis yang tidak sesuai, sehingga hidup.
Jika tiba-tiba seseorang terhubung dengan metode ini dan membaca Javadoc, mereka akan mengharapkan IllegalArgumentException , bukan ClassCastException... Saya tidak berpikir ada orang yang dengan sengaja mengalami pengecualian ini, tetapi siapa yang tahu.
Mengingat dokumentasinya, kode kemungkinan besar akan terlihat seperti ini:
public void setModel(ListModel model) {
if (!(model instanceof CheckBoxListModel)) {
throw new IllegalArgumentException(
"Model must be an instance of CheckBoxListModel!");
}
else {
super.setModel(model);
}
}
Peringatan N9
V6060 Referensi 'pemain' digunakan sebelum diverifikasi terhadap nol. 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 memperingatkan pengembang bahwa suatu objek sedang diakses sebelum diperiksa null . Pemicu aturan ini sering ditemukan dalam artikel tentang pemeriksaan proyek sumber terbuka: biasanya alasannya adalah pemfaktoran ulang yang tidak berhasil atau mengubah kontrak untuk metode. Jika Anda memperhatikan deklarasi metode getPlayer () , maka semuanya akan segera masuk ke tempatnya:
// Result must be checked for null.
// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)
Player getPlayer(UUID playerId);
Peringatan N10
V6072 Dua fragmen kode serupa ditemukan. Mungkin, ini salah ketik dan variabel 'playerB' harus digunakan sebagai pengganti '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));
}
}
}
Setelah melihat bahwa kesalahan ini ada dalam pengujian, Anda dapat segera mendevaluasi cacat yang ditemukan, dengan berpikir: "Ini adalah pengujian." Jika demikian, maka saya tidak setuju dengan Anda. Bagaimanapun, tes memainkan peran yang agak penting dalam pengembangan (meskipun tidak terlihat seperti pemrograman), dan ketika cacat muncul dalam rilis, mereka segera mulai mengarahkan jari ke tes / penguji. Jadi, tes yang rusak tidak bisa dipertahankan. Lalu mengapa tes seperti itu diperlukan? Mengapa membuang-buang sumber daya pada mereka?
Metode testArcaneAdaptationGiveType () menguji kartu "Adaptasi Arcane". Setiap pemain dibagikan kartu ke area bermain tertentu. Dan berkat copy-paste, playerA mendapatkan 2 kartu "Silvercoat Lion" yang identik di area bermain "Cemetery" , dan playerBjadi tidak ada yang didapat. Kemudian beberapa keajaiban dan pengujian sendiri.
Saat pengujian datang ke "kuburan" playerB dalam reli saat ini, maka eksekusi pengujian tidak pernah memasuki loop, karena tidak ada apa pun di "kuburan". Ini saya temukan dengan System.out.println () lama yang baik ketika memulai pengujian .
Salin-tempel yang dikoreksi:
....
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"); // <=
....
Setelah saya mengubah kodenya, ketika saya menjalankan tes, memeriksa makhluk di kuburan playerB mulai bekerja. Ave, System.out.println () !
Tes berwarna hijau sebelum dan sesudah koreksi, yang merupakan keberuntungan besar. Tetapi jika ada pengeditan yang mengubah logika eksekusi program, pengujian seperti itu akan merugikan Anda, memberi tahu Anda tentang penyelesaian yang berhasil meskipun ada kesalahan.
Salin-tempel yang sama di tempat lain:
- V6072 Dua fragmen kode serupa ditemukan. Mungkin, ini salah ketik dan variabel 'playerB' harus digunakan sebagai pengganti 'playerA'. PaintersServantTest.java (33), PaintersServantTest.java (29), PaintersServantTest.java (27), PaintersServantTest.java (31)
- V6072 Dua fragmen kode serupa ditemukan. Mungkin, ini salah ketik dan variabel 'playerB' harus digunakan sebagai pengganti 'playerA'. SubTypeChangingEffectsTest.java (32), SubTypeChangingEffectsTest.java (28), SubTypeChangingEffectsTest.java (26), SubTypeChangingEffectsTest.java (30)
Peringatan N11
V6086 Pemformatan kode yang mencurigakan. Kata kunci 'lain' mungkin hilang. 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;
}
}
Aturan diagnostik V6086 mendiagnosis pemformatan if-else-if yang salah , yang menyiratkan penghapusan else .
Potongan kode ini menunjukkan ini. Dalam kasus ini, karena ekspresi null yang dikembalikan , ketidakakuratan dalam pemformatan tidak mengarah ke apa pun, tetapi tidak masalah untuk menemukan kasus seperti itu, karena Anda tidak perlu melakukannya.
Mari kita pertimbangkan kasus di mana menghilangkan else dapat menyebabkan perilaku yang tidak terduga:
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;
}
Sekarang, dalam kasus obj == null , objek yang dimaksud akan diberi beberapa nilai, dan hal lain yang hilang akan menyebabkan objek yang baru ditetapkan mulai diperiksa di sepanjang rantai if-else-if , sementara objek seharusnya segera kembali dari metode.
Kesimpulan
Memeriksa XMage adalah artikel lain yang mengungkapkan kemampuan penganalisis statis modern. Dalam perkembangan modern, kebutuhan mereka hanya tumbuh, seiring dengan meningkatnya kompleksitas perangkat lunak. Dan tidak peduli berapa banyak rilis, pengujian, umpan balik pengguna yang Anda miliki: bug akan selalu menemukan celah untuk masuk ke basis kode Anda. Jadi mengapa tidak menambahkan penghalang lain untuk pertahanan Anda?
Seperti yang Anda pahami, penganalisis rentan terhadap kesalahan positif (termasuk PVS-Studio Java). Ini bisa jadi akibat dari kesalahan yang jelas dan kode yang terlalu membingungkan (sayangnya, penganalisis tidak mengetahuinya). Anda perlu memperlakukan mereka dengan pengertian dan segera berhenti berlangganan tanpa ragu-ragu , dan sementara positif palsu menunggu koreksi mereka, Anda dapat menggunakan salah satu metodemenekan peringatan.
Sebagai kesimpulan, saya sarankan Anda secara pribadi "menyentuh" penganalisis dengan mengunduhnya dari situs web kami.

Jika Anda ingin berbagi artikel ini dengan audiens berbahasa Inggris, silakan gunakan tautan terjemahan: Maxim Stefanov. Memeriksa Kode XMage, dan Mengapa Anda Tidak Akan Bisa Mendapatkan Kartu Langka Khusus Koleksi Labirin Naga .