Artikel ini dikhususkan untuk memeriksa proyek OpenRA menggunakan penganalisis statis PVS-Studio. Apa itu OpenRA? Ini adalah mesin game open source untuk membuat game strategi waktu nyata. Artikel tersebut menjelaskan bagaimana analisis dilakukan, fitur apa dari proyek itu sendiri yang ditemukan dan pemicu menarik apa yang diberikan PVS-Studio. Dan, tentu saja, di sini kita akan mempertimbangkan beberapa fitur penganalisis yang membuat proses verifikasi proyek menjadi lebih nyaman.
OpenRA
Proyek yang dipilih untuk ditinjau adalah mesin game RTS dengan gaya game seperti Command & Conquer: Red Alert. Informasi lebih lanjut dapat ditemukan di situs web . Kode sumber ditulis dalam C # dan tersedia untuk dilihat dan digunakan dalam repositori .
OpenRA dipilih untuk ditinjau karena 3 alasan. Pertama, tampaknya menarik bagi banyak orang. Bagaimanapun, ini berlaku untuk penghuni GitHub, karena repositori telah mengumpulkan lebih dari 8 ribu bintang. Kedua, basis kode OpenRA berisi 1285 file. Biasanya jumlah ini cukup untuk diharapkan menemukan pemicu yang menarik di dalamnya. Dan ketiga ... Mesin game itu keren.
Positif ekstra
Saya menganalisis OpenRA menggunakan PVS-Studio dan awalnya terinspirasi oleh hasilnya:
Saya memutuskan bahwa di antara begitu banyak Peringatan Tinggi, saya pasti dapat menemukan banyak sekali tanggapan yang sangat berbeda. Dan, tentu saja, atas dasar mereka saya akan menulis artikel paling keren dan paling menarik. Tapi itu tidak ada!
Seseorang hanya perlu melihat peringatan itu sendiri, dan semuanya segera jatuh ke tempatnya. Dari 1306 peringatan Tinggi, 1.277 dikaitkan dengan diagnostik V3144 . Ini menampilkan pesan seperti "File ini ditandai dengan lisensi copyleft, yang mengharuskan Anda untuk membuka kode sumber turunan". Diagnostik ini dijelaskan lebih detail di sini .
Jelas, cara kerja rencana seperti itu sama sekali tidak menarik minat saya, karena OpenRA sudah merupakan proyek open-source. Oleh karena itu, mereka perlu disembunyikan agar tidak mengganggu tampilan log lainnya. Karena saya menggunakan plugin untuk Visual Studio, itu mudah dilakukan. Anda hanya perlu mengeklik kanan salah satu peringatan V3144 dan memilih "Sembunyikan semua kesalahan V3144 " di menu yang terbuka .
Anda juga dapat memilih peringatan mana yang akan ditampilkan di log dengan membuka bagian "Kesalahan yang Dapat Dideteksi (C #)" di opsi penganalisis.
Untuk pergi ke mereka menggunakan plugin untuk Visual Studio 2019, Anda perlu mengklik menu bagian atas Extensions-> PVS-Studio-> Options.
Hasil tes
Setelah pemicu V3144 difilter, ada lebih sedikit peringatan di log:
Meski demikian, kami berhasil menemukan momen menarik di antara mereka.
Kondisi tidak berarti
Beberapa pemicu menunjukkan pemeriksaan yang tidak perlu. Ini mungkin menunjukkan kesalahan, karena biasanya orang tidak menulis kode seperti itu dengan sengaja. Namun, di OpenRA sering kali kondisi yang tidak perlu ini sengaja ditambahkan. Misalnya:
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}Peringatan Analyzer : Ekspresi V3022 'Aktif' selalu benar. SupportPowerManager.cs 206
PVS-Studio cukup benar catatan bahwa cek kedua tidak berarti, karena jika aktif adalah palsu , maka eksekusi akan tidak mencapai itu. Ini mungkin kesalahan, tapi saya pikir itu ditulis dengan sengaja. Untuk apa? Nah, kenapa tidak?
Mungkin di hadapan kita kita memiliki semacam solusi sementara, yang revisinya ditinggalkan "untuk nanti". Dalam kasus seperti itu, akan lebih mudah bahwa penganalisis akan mengingatkan pengembang tentang ketidaksempurnaan tersebut.
Mari kita lihat satu lagi pemeriksaan "untuk berjaga-jaga":
Pair<string, bool>[] MakeComponents(string text)
{
....
if (highlightStart > 0 && highlightEnd > highlightStart) // <=
{
if (highlightStart > 0) // <=
{
// Normal line segment before highlight
var lineNormal = line.Substring(0, highlightStart);
components.Add(Pair.New(lineNormal, false));
}
// Highlight line segment
var lineHighlight = line.Substring(
highlightStart + 1,
highlightEnd - highlightStart – 1
);
components.Add(Pair.New(lineHighlight, true));
line = line.Substring(highlightEnd + 1);
}
else
{
// Final normal line segment
components.Add(Pair.New(line, false));
break;
}
....
}Peringatan Analyzer : V3022 Expression 'highlightStart> 0' selalu benar. LabelWithHighlightWidget.cs 54
Sekali lagi, jelas bahwa pemeriksaan ulang sama sekali tidak ada artinya. Nilai highlightStart dicentang dua kali di baris yang berdekatan. Kesalahan? Ada kemungkinan bahwa dalam salah satu kondisi, variabel yang salah dipilih untuk pengujian. Either way, sulit untuk mengatakan dengan pasti tentang apa ini. Satu hal yang jelas - kode perlu dipelajari dan diperbaiki, atau penjelasan harus ditinggalkan jika pemeriksaan tambahan masih diperlukan karena alasan tertentu.
Inilah poin serupa lainnya:
public static void ButtonPrompt(....)
{
....
var cancelButton = prompt.GetOrNull<ButtonWidget>(
"CANCEL_BUTTON"
);
....
if (onCancel != null && cancelButton != null)
{
cancelButton.Visible = true;
cancelButton.Bounds.Y += headerHeight;
cancelButton.OnClick = () =>
{
Ui.CloseWindow();
if (onCancel != null)
onCancel();
};
if (!string.IsNullOrEmpty(cancelText) && cancelButton != null)
cancelButton.GetText = () => cancelText;
}
....
}Peringatan penganalisis : V3063 Bagian dari ekspresi bersyarat selalu benar jika dievaluasi: cancelButton! = Null. ConfirmationDialogs.cs 78
cancelButton memang bisa nol , karena nilai yang dikembalikan oleh metode GetOrNull ditulis ke variabel ini . Namun, logis untuk mengasumsikan bahwa cancelButton dalam badan operator bersyarat tidak akan menjadi null dengan cara apa pun . Meski demikian, masih ada pemeriksaan. Jika Anda tidak memperhatikan kondisi eksternal, maka secara umum situasi yang aneh ternyata: pertama, properti variabel diakses, dan kemudian pengembang memutuskan untuk memastikan - apakah masih null atau tidak?
Pada awalnya, saya berasumsi bahwa proyek tersebut mungkin menggunakan beberapa logika khusus yang terkait dengan membebani operator "==". Menurut pendapat saya, menerapkan sesuatu yang serupa untuk jenis referensi dalam sebuah proyek adalah ide yang kontroversial. Namun, perilaku yang tidak biasa membuat developer lain lebih sulit untuk memahami kode tersebut. Pada saat yang sama, sulit bagi saya untuk membayangkan situasi ketika trik seperti itu tidak dapat diabaikan. Meskipun kemungkinan besar dalam beberapa kasus tertentu, ini akan menjadi solusi yang nyaman.
Di mesin game Unity, misalnya, operator " == " didefinisikan ulang untuk kelas UnityEngine.Object . Dokumentasi resmi yang tersedia di tautan menunjukkan bahwa membandingkan contoh kelas ini dengan nulltidak berfungsi seperti biasa. Nah, tentunya para pengembang punya alasan untuk menerapkan logika yang tidak biasa tersebut.
Saya tidak menemukan yang seperti itu di OpenRA :). Jadi jika ada arti dalam pemeriksaan yang dianggap sebelumnya untuk null , maka itu terdiri dari sesuatu yang lain.
PVS-Studio dapat mendeteksi beberapa momen serupa, tetapi tidak perlu mencantumkan semuanya di sini. Masih membosankan untuk melihat hal positif yang sama. Untungnya (atau tidak) penganalisis dapat menemukan keanehan lain juga.
Kode tidak dapat dijangkau
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}Peringatan Analyzer : Ekspresi V3022 '! Order.Queued && currentTransform.NextActivity! = Null' selalu salah. TransformsIntoTransforms.cs 44
Sekali lagi kami memiliki pemeriksaan yang tidak berguna. Benar, berbeda dengan yang sebelumnya, di sini disajikan bukan hanya kondisi tambahan, tetapi kode yang paling tidak terjangkau yang paling nyata. Pemeriksaan yang dianggap selalu benar sebelumnya ternyata tidak mempengaruhi pengoperasian program. Mereka dapat dihapus dari kode, atau mereka dapat ditinggalkan - tidak ada yang akan berubah.
Di sini, pemeriksaan aneh mengarah pada fakta bahwa sebagian kode tidak dieksekusi. Pada saat yang sama, sulit bagi saya untuk menebak perubahan apa yang harus dilakukan di sini sebagai amandemen. Dalam kasus yang paling sederhana dan menyenangkan, kode yang tidak dapat dijangkau tidak boleh dijalankan. Maka tidak ada kesalahan. Namun, saya ragu programmer tersebut sengaja menulis kalimat tersebut hanya untuk kecantikan.
Variabel yang tidak diinisialisasi dalam konstruktor
public class CursorSequence
{
....
public readonly ISpriteFrame[] Frames;
public CursorSequence(
FrameCache cache,
string name,
string cursorSrc,
string palette,
MiniYaml info
)
{
var d = info.ToDictionary();
Start = Exts.ParseIntegerInvariant(d["Start"].Value);
Palette = palette;
Name = name;
if (
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")
)
Length = Frames.Length - Start;
else if (d.ContainsKey("Length"))
Length = Exts.ParseIntegerInvariant(d["Length"].Value);
else if (d.ContainsKey("End"))
Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
else
Length = 1;
Frames = cache[cursorSrc]
.Skip(Start)
.Take(Length)
.ToArray();
....
}
}Peringatan Analyzer : V3128 Bidang 'Frames' digunakan sebelum diinisialisasi di konstruktor. CursorSequence.cs 35 Momen yang
sangat tidak menyenangkan. Mencoba mendapatkan nilai properti Length dari variabel yang tidak diinisialisasi akan memunculkan NullReferenceException . Dalam situasi normal, kesalahan seperti itu hampir tidak akan luput dari perhatian - namun, ketidakmungkinan membuat instance kelas dengan mudah terungkap. Tapi disini pengecualian hanya akan dilempar jika kondisinya
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")akan menjadi kenyataan.
Sulit untuk menilai bagaimana Anda perlu memperbaiki kode agar semuanya bekerja dengan baik. Saya hanya dapat berasumsi bahwa fungsinya akan terlihat seperti ini:
public CursorSequence(....)
{
var d = info.ToDictionary();
Start = Exts.ParseIntegerInvariant(d["Start"].Value);
Palette = palette;
Name = name;
ISpriteFrame[] currentCache = cache[cursorSrc];
if (
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")
)
Length = currentCache.Length - Start;
else if (d.ContainsKey("Length"))
Length = Exts.ParseIntegerInvariant(d["Length"].Value);
else if (d.ContainsKey("End"))
Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
else
Length = 1;
Frames = currentCache
.Skip(Start)
.Take(Length)
.ToArray();
....
}Dalam versi ini, masalah yang ditentukan tidak ada, namun, hanya pengembang yang dapat mengatakan seberapa sesuai dengan ide aslinya.
Potensi kesalahan ketik
public void Resize(int width, int height)
{
var oldMapTiles = Tiles;
var oldMapResources = Resources;
var oldMapHeight = Height;
var oldMapRamp = Ramp;
var newSize = new Size(width, height);
....
Tiles = CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
Resources = CellLayer.Resize(
oldMapResources,
newSize,
oldMapResources[MPos.Zero]
);
Height = CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
Ramp = CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);
....
}Analyzer Warning : V3127 Dua fragmen kode serupa ditemukan. Mungkin ini salah ketik dan variabel 'oldMapRamp' harus digunakan sebagai ganti 'oldMapHeight' Map.cs 964
Penganalisis telah mendeteksi momen mencurigakan terkait dengan meneruskan argumen ke fungsi. Mari kita lihat panggilan secara terpisah:
CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
CellLayer.Resize(oldMapResources, newSize, oldMapResources[MPos.Zero]);
CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);Anehnya, panggilan terakhir melewati oldMapHeight , bukan oldMapRamp . Tentu saja, tidak semua kasus seperti itu benar-benar kesalahan. Sangat mungkin bahwa semuanya ditulis dengan benar di sini. Tapi harus Anda akui kalau tempat ini terlihat tidak biasa. Saya cenderung percaya bahwa kesalahan memang telah dibuat di sini.
Catatan dari seorang kolega - Andrey Karpov . Dan saya tidak melihat sesuatu yang aneh dalam kode ini. Ini adalah kesalahan baris terakhir klasik !
Jika masih tidak ada kesalahan di sini, maka ada baiknya menambahkan beberapa penjelasan. Lagi pula, jika suatu saat terlihat seperti kesalahan, maka seseorang pasti ingin memperbaikinya.
Benar, benar dan tidak lain adalah benar
Proyek ini berisi metode yang sangat aneh, yang nilai kembaliannya memiliki tipe bool . Keunikan mereka terletak pada kenyataan bahwa dalam kondisi apa pun mereka kembali benar . Misalnya:
static bool State(
S server,
Connection conn,
Session.Client client,
string s
)
{
var state = Session.ClientState.Invalid;
if (!Enum<Session.ClientState>.TryParse(s, false, out state))
{
server.SendOrderTo(conn, "Message", "Malformed state command");
return true;
}
client.State = state;
Log.Write(
"server",
"Player @{0} is {1}",
conn.Socket.RemoteEndPoint,
client.State
);
server.SyncLobbyClients();
CheckAutoStart(server);
return true;
}Peringatan Analyzer : V3009 Aneh bahwa metode ini selalu mengembalikan satu dan nilai yang sama dari 'true'. LobbyCommands.cs 123
Apakah kode ini oke? Apakah ada kesalahan Ini terlihat sangat aneh. Mengapa developer tidak menggunakan void ?
Tidaklah mengherankan jika penganalisis menganggap tempat seperti itu aneh, tetapi kami tetap harus mengakui bahwa pemrogram sebenarnya punya alasan untuk menulis dengan cara ini. Yang mana?
Saya memutuskan untuk melihat di mana metode ini dipanggil dan apakah nilai kembaliannya selalu benar yang digunakan. Ternyata hanya ada satu referensi untuk itu di kelas yang sama - di kamus commandHandlers , yang memiliki tipe
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>Selama inisialisasi, nilai ditambahkan ke dalamnya
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}dll.
Kami disajikan dengan kasus langka (saya ingin mempercayainya) ketika pengetikan statis menciptakan masalah bagi kami. Lagi pula, membuat kamus yang nilainya akan berfungsi dengan tanda tangan yang berbeda ... setidaknya bermasalah. commandHandlers hanya digunakan dalam metode InterpretCommand :
public bool InterpretCommand(
S server, Connection conn, Session.Client client, string cmd
)
{
if (
server == null ||
conn == null ||
client == null ||
!ValidateCommand(server, conn, client, cmd)
) return false;
var cmdName = cmd.Split(' ').First();
var cmdValue = cmd.Split(' ').Skip(1).JoinWith(" ");
Func<S, Connection, Session.Client, string, bool> a;
if (!commandHandlers.TryGetValue(cmdName, out a))
return false;
return a(server, conn, client, cmdValue);
}Rupanya, tujuan pengembang adalah kemampuan universal untuk mencocokkan string dari operasi tertentu yang dilakukan. Saya pikir cara dia memilih jauh dari satu-satunya, namun, tidak mudah untuk menawarkan sesuatu yang lebih nyaman / benar dalam situasi seperti itu. Terutama jika Anda tidak menggunakan semacam dinamika atau yang serupa. Jika Anda memiliki ide tentang hal ini, tinggalkan komentar. Akan menarik bagi saya untuk melihat berbagai opsi untuk memecahkan masalah ini.
Ternyata peringatan yang terkait dengan metode selalu benar di kelas ini kemungkinan besar salah. Namun ... "Kemungkinan besar" inilah yang membuat Anda takut. Anda perlu berhati-hati dengan hal-hal seperti itu, karena mungkin memang ada kesalahan di antara mereka.
Semua hal positif tersebut harus diperiksa dengan cermat dan kemudian ditandai sebagai salah jika perlu. Ini dilakukan dengan cukup sederhana. Komentar khusus harus ditinggalkan di tempat yang ditunjukkan oleh penganalisis:
static bool State(....) //-V3009Ada cara lain: Anda dapat memilih positif yang perlu ditandai sebagai salah, dan di menu konteks, klik "Tandai pesan yang dipilih sebagai Alarm Salah".
Detail lebih lanjut dapat dieksplorasi topik ini di dokumentasi .
Pemeriksaan ekstra untuk nol?
static bool SyncLobby(....)
{
if (!client.IsAdmin)
{
server.SendOrderTo(conn, "Message", "Only the host can set lobby info");
return true;
}
var lobbyInfo = Session.Deserialize(s);
if (lobbyInfo == null) // <=
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}
server.LobbyInfo = lobbyInfo;
server.SyncLobbyInfo();
return true;
}Peringatan penganalisis : Ekspresi V3022 'lobbyInfo == null' selalu salah. LobbyCommands.cs 851
Metode lain yang selalu mengembalikan nilai true . Namun, kali ini kami memeriksa jenis pemicu yang berbeda. Anda perlu mempelajari hal-hal seperti itu dengan cukup hati-hati, karena ini jauh dari fakta bahwa ini hanya kode yang berlebihan. Tapi hal pertama yang pertama.
Metode Deserialize tidak pernah mengembalikan nol - Anda dapat dengan mudah memverifikasi ini dengan melihat kodenya:
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}Untuk keterbacaan, saya telah mempersingkat kode sumber dari metode ini. Anda dapat melihatnya secara lengkap dengan mengikuti tautannya . Yah, atau percayalah bahwa variabel sesi di sini dalam keadaan apa pun tidak berubah menjadi nol .
Apa yang kita lihat di bawah? Deserialize tidak mengembalikan null , jika terjadi kesalahan, ia akan mengeluarkan pengecualian. Pengembang yang menulis cek null setelah panggilan tersebut tampaknya berpikir secara berbeda. Kemungkinan besar, dalam situasi luar biasa, metode SyncLobby harus mengeksekusi kode yang saat ini sedang dieksekusi ... tetapi tidak pernah dijalankan, karena lobbyInfo tidak pernah null :
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}Saya percaya bahwa alih-alih pemeriksaan "ekstra" ini, Anda masih harus menggunakan coba - tangkap . Nah, atau pergi dari sisi lain dan tulis beberapa TryDeserialize , yang dalam kasus pengecualian akan mengembalikan null .
Kemungkinan NullReferenceException
public ConnectionSwitchModLogic(....)
{
....
var logo = panel.GetOrNull<RGBASpriteWidget>("MOD_ICON");
if (logo != null)
{
logo.GetSprite = () =>
{
....
};
}
if (logo != null && mod.Icon == null) // <=
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width; // <=
}
....
}Peringatan Analyzer : V3125 Objek 'logo' digunakan setelah diverifikasi terhadap null. Periksa baris: 236, 222. ConnectionLogic.cs 236
Sesuatu memberitahu saya bahwa ada kesalahan 100% di sini. Kami jelas tidak memiliki pemeriksaan "ekstra" sebelum kami, karena metode GetOrNull, kemungkinan besar, benar-benar dapat mengembalikan referensi null. Apa yang terjadi jika logo tersebut nol ? Memanggil properti Bounds akan mengeluarkan pengecualian, yang jelas tidak ada dalam rencana pengembang.
Mungkin fragmen perlu ditulis ulang seperti ini:
if (logo != null)
{
if (mod.Icon == null)
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width;
}
}Opsi ini cukup sederhana untuk dipahami, meskipun penumpukan tambahan tidak terlihat sangat keren. Sebagai solusi yang lebih luas, Anda dapat menggunakan operator bersyarat null:
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=Perhatikan bahwa saya lebih suka perbaikan atas. Sangat menyenangkan untuk membacanya dan tidak ada pertanyaan yang muncul. Tetapi beberapa pengembang sangat menghargai keringkasan, jadi saya memutuskan untuk memberikan opsi kedua juga :).
Mungkinkah itu OrDefault?
public MapEditorLogic(....)
{
var editorViewport = widget.Get<EditorViewportControllerWidget>("MAP_EDITOR");
var gridButton = widget.GetOrNull<ButtonWidget>("GRID_BUTTON");
var terrainGeometryTrait = world.WorldActor.Trait<TerrainGeometryOverlay>();
if (gridButton != null && terrainGeometryTrait != null) // <=
{
....
}
var copypasteButton = widget.GetOrNull<ButtonWidget>("COPYPASTE_BUTTON");
if (copypasteButton != null)
{
....
}
var copyFilterDropdown = widget.Get<DropDownButtonWidget>(....);
copyFilterDropdown.OnMouseDown = _ =>
{
copyFilterDropdown.RemovePanel();
copyFilterDropdown.AttachPanel(CreateCategoriesPanel());
};
var coordinateLabel = widget.GetOrNull<LabelWidget>("COORDINATE_LABEL");
if (coordinateLabel != null)
{
....
}
....
}Peringatan penganalisis : V3063 Bagian dari ekspresi bersyarat selalu benar jika dievaluasi: terrainGeometryTrait! = Null. MapEditorLogic.cs 35
Mari kita menganalisis cuplikan ini. Perhatikan bahwa setiap kali metode GetOrNull kelas Widget digunakan , null akan diperiksa . Namun, jika Get digunakan , tidak ada validasinya. Ini logis - metode Get tidak mengembalikan null :
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}Jika elemen tidak ditemukan, pengecualian akan dilemparkan - ini adalah perilaku yang wajar. Dan pada saat yang sama, opsi logisnya adalah memeriksa nilai yang dikembalikan oleh metode GetOrNull untuk persamaan ke referensi null.
Dalam kode di atas, nilai yang dikembalikan oleh metode Trait diperiksa untuk null . Faktanya, di dalam metode Trait , Get dipanggil dari kelas TraitDictionary :
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}Mungkinkah Get ini berperilaku berbeda dari yang dibahas sebelumnya? Namun kelasnya berbeda. Mari kita periksa:
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}Metode InnerGet mengembalikan turunan TraitContainer <T> . Implementasi Get di kelas ini sangat mirip dengan Get di kelas Widget :
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}Kesamaan utama adalah bahwa null tidak pernah dikembalikan ke sini . Jika ada yang tidak beres, InvalidOperationException akan muncul dengan cara serupa . Oleh karena itu, metode Trait berperilaku dengan cara yang sama.
Ya, mungkin hanya ada pemeriksaan tambahan di sini, yang tidak memengaruhi apa pun. Mungkin terlihat aneh, tetapi kami tidak dapat mengatakan bahwa kode seperti itu akan sangat membingungkan pembaca. Tetapi jika pemeriksaan hanya diperlukan di sini, maka dalam beberapa kasus pengecualian akan muncul secara tidak terduga. Sedih.
Pada titik ini, tampaknya lebih tepat untuk memanggil semacam TraitOrNull . Namun, tidak ada metode seperti itu :). Tapi ada TraitOrDefault , yang mirip dengan GetOrNullUntuk kasus ini.
Ada poin serupa lainnya yang terkait dengan metode Dapatkan :
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}Peringatan Analyzer : V3022 Expression 'frameSlider! = Null' selalu benar. AssetBrowserLogic.cs 128
Seperti pada kode di atas , ada yang tidak beres di sini. Entah pemeriksaan tersebut benar-benar tidak diperlukan, atau sebagai ganti Get , Anda masih perlu memanggil GetOrNull .
Tugas hilang
public SpawnSelectorTooltipLogic(....)
{
....
var textWidth = ownerFont.Measure(labelText).X;
if (textWidth != cachedWidth)
{
label.Bounds.Width = textWidth;
widget.Bounds.Width = 2 * label.Bounds.X + textWidth; // <=
}
widget.Bounds.Width = Math.Max( // <=
teamWidth + 2 * labelMargin,
label.Bounds.Right + labelMargin
);
team.Bounds.Width = widget.Bounds.Width;
....
}Peringatan Analyzer : V3008 Variabel 'widget.Bounds.Width' diberi nilai dua kali berturut-turut. Mungkin ini salah. Baris centang: 78, 75. SpawnSelectorTooltipLogic.cs 78
Sepertinya jika kondisi textWidth! = CachedWidth benar , beberapa nilai khusus kasus harus ditulis ke widget.Bounds.Width . Namun, penetapan di bawah, terlepas dari apakah kondisi ini benar, menghilangkan string
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;setiap perasaan. Kemungkinan mereka hanya lupa meletakkan yang lain di sini :
if (textWidth != cachedWidth)
{
label.Bounds.Width = textWidth;
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;
}
else
{
widget.Bounds.Width = Math.Max(
teamWidth + 2 * labelMargin,
label.Bounds.Right + labelMargin
);
}Memeriksa nilai default
public void DisguiseAs(Actor target)
{
....
var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
AsPlayer = tooltip.Owner;
AsActor = target.Info;
AsTooltipInfo = tooltip.TooltipInfo;
....
}Peringatan Analyzer : V3146 Kemungkinan dereferensi null dari 'tooltip'. 'FirstOrDefault' dapat mengembalikan nilai nol default. Disguise.cs 192
Kapan FirstOrDefault biasa digunakan sebagai pengganti First ? Jika pilihannya kosong, First akan memunculkan InvalidOperationException . FirstOrDefault tidak akan memunculkan pengecualian, tetapi akan mengembalikan null untuk jenis referensi.
Dalam proyek tersebut, antarmuka ITooltip diimplementasikan oleh berbagai kelas. Jadi, jika target.TraitsImplementing <ITooltii> () mengembalikan pilihan kosong, tooltip akan ditulis null... Akses lebih lanjut ke properti objek ini akan menghasilkan NullReferenceException .
Dalam kasus di mana pengembang yakin bahwa pilihan tidak akan kosong, lebih tepat menggunakan First . Jika Anda tidak begitu yakin, maka Anda harus memeriksa nilai yang dikembalikan oleh FirstOrDefault. Anehnya, ini tidak ada di sini. Bagaimanapun, nilai yang dikembalikan oleh metode GetOrNull yang telah dibahas sebelumnya selalu diperiksa. Mengapa mereka tidak melakukannya di sini?
Siapa tahu ... Oh, tepatnya! Tentunya pengembang dapat menjawab pertanyaan ini. Pada akhirnya, dia harus mengedit kode ini.
Kesimpulan
OpenRA ternyata menjadi proyek yang menyenangkan dan menarik untuk dilihat. Para pengembang melakukan pekerjaan dengan baik, dan pada saat yang sama tidak lupa bahwa sumbernya harus mudah dipelajari. Tentu saja, di sini ada poin-poin kontroversial yang berbeda, tetapi di mana tanpa mereka.
Pada saat yang sama, bahkan dengan semua upaya mereka, pengembang (sayangnya) tetaplah manusia. Beberapa positif yang dianggap sangat sulit untuk diperhatikan tanpa menggunakan penganalisis. Terkadang sulit untuk menemukan kesalahan bahkan segera setelah menulisnya. Apa yang dapat kami katakan tentang menemukan mereka setelah sekian lama.
Jelas, mendeteksi kesalahan jauh lebih baik daripada konsekuensinya. Untuk ini, Anda dapat menghabiskan waktu berjam-jam untuk memeriksa ulang sejumlah besar sumber baru secara manual. Nah, yang lama pada saat yang sama terlihat - tiba-tiba tidak menyadari adanya kesalahan sebelumnya? Ya, ulasan sangat berguna, tetapi jika Anda harus melihat banyak kode, lama kelamaan Anda akan berhenti memperhatikan beberapa hal. Dan banyak waktu dan tenaga dihabiskan untuk itu.
Analisis statis hanyalah tambahan yang nyaman untuk metode lain untuk memeriksa kualitas kode sumber, seperti peninjauan kode. PVS-Studio akan menemukan kesalahan "sederhana" (dan terkadang tidak hanya) daripada pengembang, yang memungkinkan orang untuk fokus pada masalah yang lebih serius.
Ya, penganalisis terkadang menghasilkan positif palsu dan tidak dapat menemukan semua kesalahan sama sekali. Tetapi menggunakannya akan menghemat banyak waktu dan saraf. Ya, dia tidak sempurna dan terkadang dia membuat kesalahan sendiri. Namun secara umum, PVS-Studio membuat proses pengembangan menjadi lebih mudah, menyenangkan dan bahkan (tidak terduga!) Lebih murah.
Sebenarnya, Anda tidak perlu mengambil kata-kata saya untuk itu - akan jauh lebih baik untuk memverifikasi sendiri kebenaran di atas. Ikuti tautan untuk mengunduh penganalisis dan mendapatkan kunci uji coba. Seberapa mudah?
Nah, itu saja. Terima kasih atas perhatian Anda! Saya berharap Anda membersihkan kode dan log kesalahan bersih yang sama!
Jika Anda ingin berbagi artikel ini dengan audiens berbahasa Inggris, silakan gunakan tautan terjemahan: Nikita Lipilin. Unicorn membobol RTS: menganalisis kode sumber OpenRA .