眾所周知，在團(tuán)隊(duì)中進(jìn)行代碼審查（Code Review）可以提升代碼質(zhì)量，分享項(xiàng)目知識(shí)、明確責(zé)任，最終達(dá)到構(gòu)建更好的軟件、更好的團(tuán)隊(duì)。如果你花幾秒鐘搜索代碼審查的相關(guān)信息，你會(huì)看到許多關(guān)于代碼審查帶來的價(jià)值的文章。也有許多方法來進(jìn)行代碼審查：在GitHub中提pull request，或使用像JetBrains的Upsource之類的工具。然而即使擁有清晰的流程和正確的工具，還遺留了一個(gè)大問題需要解決——我們需要找尋哪些問題。
可能沒有明確關(guān)于“我們需要找尋哪些問題”的文章，是因?yàn)橛性S多不同的要點(diǎn)需要考慮。正如任何其他的需求，各個(gè)團(tuán)隊(duì)對(duì)各個(gè)方面都有不同的優(yōu)先級(jí)。
本文的目標(biāo)是列出一些審查者可以找尋的要點(diǎn)，而各個(gè)方面的優(yōu)先級(jí)就因各個(gè)團(tuán)隊(duì)而異了。
在我們繼續(xù)之前，讓我們考慮一下大家在代碼審查時(shí)會(huì)討論到的問題。對(duì)于代碼的格式、樣式和命名以及缺少單元測(cè)試這些問題是很常見的幾點(diǎn)。
如果你想擁有可持續(xù)的、可維護(hù)的代碼，這些是有用的檢查點(diǎn)。然而，在代碼審查時(shí)討論這些就有些浪費(fèi)時(shí)間，因?yàn)楹芏噙@樣的檢查可以（也應(yīng)該）被自動(dòng)化。
那哪些要點(diǎn)是只能由人工進(jìn)行審查而不能依靠工具的呢？回答是有驚人數(shù)量的點(diǎn)只能由人工進(jìn)行審查, 以下：
- 如何讓新代碼與全局的架構(gòu)保持一致？
- 代碼是否遵循SOLID原則？
- 是否遵循團(tuán)隊(duì)使用的設(shè)計(jì)規(guī)范，如領(lǐng)域驅(qū)動(dòng)開發(fā)等？
- 新代碼使用了什么設(shè)計(jì)模式？這樣使用是否合適？
- 基礎(chǔ)代碼是否有結(jié)合使用了一些標(biāo)準(zhǔn)或設(shè)計(jì)樣式？
- 新的代碼是否遵循當(dāng)前的規(guī)范？
- 代碼是否正確遷移，或參照了因不規(guī)范而淘汰的舊代碼？
- 代碼的位置是否正確？比如涉及訂單的新代碼是否在訂單服務(wù)相關(guān)的位置？
- 新代碼是否重用了現(xiàn)存的代碼？
- 新代碼是否可以被現(xiàn)有代碼重用？
- 新代碼是否有重復(fù)代碼？如果是的話，是否應(yīng)該重構(gòu)成一個(gè)更可被重用的模式，還是當(dāng)前還可以接受？
- 新代碼是否被過度設(shè)計(jì)了？是否引入現(xiàn)在還不需要的重用設(shè)計(jì)？團(tuán)隊(duì)如何平衡可重用和YAGNI(You Ain’t Gonna Need It)這兩種觀點(diǎn)？
- 字段、變量、參數(shù)、方法、類的命名是否真實(shí)反映它們所代表的事物。
- 我是否可以通過讀代碼理解它做了什么？
- 我是否理解測(cè)試用例測(cè)了什么？
- 測(cè)試是否很好地覆蓋了用例的各種情況？它們是否覆蓋了正常和異常用例？是否有忽略的情況？
- 錯(cuò)誤信息是否可被理解？
- 不清晰的代碼是否被文檔、注釋或容易理解的測(cè)試用例所覆蓋？具體可以根據(jù)團(tuán)隊(duì)自身的喜好決定使用哪種方式。
- 代碼是否真的達(dá)到了預(yù)期的目標(biāo)？如果有自動(dòng)化測(cè)試來確保代碼的正確性，測(cè)試的代碼是否真的可以驗(yàn)證代碼達(dá)到了協(xié)定的需求？
- 代碼看上去是否包含不明顯的bug，比如使用錯(cuò)誤的變量進(jìn)行檢查，或誤把a(bǔ)nd寫成or？
-是否需要滿足相關(guān)監(jiān)管需求？ - 作者是否需要?jiǎng)?chuàng)建公共文檔或修改現(xiàn)存的幫助文檔？
- 是否檢查了面向用戶的信息的正確性？
- 是否有會(huì)在生產(chǎn)環(huán)境中導(dǎo)致應(yīng)用停止運(yùn)行的明顯錯(cuò)誤？代碼是否會(huì)錯(cuò)誤地指向測(cè)試數(shù)據(jù)庫(kù)，是否存在應(yīng)在真實(shí)服務(wù)中移除的硬編碼的stub代碼？
在本文剩下的部分，我們會(huì)覆蓋一系列廣泛的特性，并深入其中的兩點(diǎn)具體的區(qū)域：性能和安全。
你對(duì)性能的需求是什么，你是否考慮了安全問題？這些是需要覆蓋到的重大區(qū)域也是至關(guān)重要的話題。
讓我們深入探討下性能，這是一個(gè)真正能從代碼審查中獲益的方面。
系統(tǒng)對(duì)性能方面的非功能性需求應(yīng)當(dāng)同所有架構(gòu)、設(shè)計(jì)的領(lǐng)域一樣被置于重要位置。無論你是開發(fā)只容許納秒級(jí)延時(shí)的低延遲交易系統(tǒng)，還是管理“待辦事項(xiàng)”的手機(jī)應(yīng)用，你都應(yīng)該了解用戶所認(rèn)為的“太慢”。
在考慮我們是否需要就代碼性能進(jìn)行代碼審查之前，我們應(yīng)該問自己幾個(gè)關(guān)于具體需求是什么的問題。雖然一些應(yīng)用確實(shí)不需要考慮每毫秒都花費(fèi)在哪里，對(duì)于大部分應(yīng)用，花費(fèi)幾個(gè)小時(shí)的折騰進(jìn)行優(yōu)化來獲得的些許CPU下降的價(jià)值也是有限的，但有些地方還是審查者可以檢查一下的，進(jìn)而確保代碼不會(huì)有一些常見可避免的性能缺陷。
這段代碼是否有硬性的性能需求？
接受審查的代碼是否涉及之前發(fā)布的服務(wù)等級(jí)協(xié)議（SLA）？或這個(gè)需求本身有特別的性能需求？
如果代碼導(dǎo)致“登錄頁面加載太慢”，那原始的開發(fā)者需要找出合適的加載時(shí)間是多久，不然審查者或作者本人如何確保改進(jìn)后的速度足夠快？
如果有硬性的需求，是否有測(cè)試能證明滿足了該需求？任何注重性能的系統(tǒng)應(yīng)該就性能提供自動(dòng)化測(cè)試，這能確保發(fā)布的SLA達(dá)到預(yù)期（如所有訂單請(qǐng)求要在10毫秒內(nèi)處理）。沒有這些，你只能依靠你的用戶來告訴你沒有達(dá)到對(duì)應(yīng)的SLA。這不僅是一種糟糕的用戶體驗(yàn)，還會(huì)帶來原本可避免的罰金和支出。
這個(gè)修復(fù)或新增的功能是否會(huì)反向影響到任何現(xiàn)存的性能測(cè)試結(jié)果
如果你定期運(yùn)行性能測(cè)試或有測(cè)試套件可以按需運(yùn)行它們，那你就需要檢查新的代碼是否使得性能關(guān)鍵區(qū)域的系統(tǒng)性能有所下降。這可以是一個(gè)自動(dòng)化的流程，但由于在持續(xù)集成環(huán)境中更常運(yùn)行單元測(cè)試而不是性能測(cè)試，所以值得特別指出可以在代碼審查中檢查這項(xiàng)。
調(diào)用外部的服務(wù)或應(yīng)用的代價(jià)是昂貴的
任何通過網(wǎng)路來使用外部系統(tǒng)的方式通常會(huì)比沒有很好優(yōu)化的方法有更差的性能?？紤]以下幾點(diǎn)：
調(diào)用數(shù)據(jù)庫(kù)：最壞的情況是問題隱藏在系統(tǒng)抽象中，如關(guān)系對(duì)象映射（ORM）中。但是在代碼審查中你應(yīng)該可以找到常見的導(dǎo)致性能問題的問題，如在循環(huán)中逐個(gè)調(diào)用數(shù)據(jù)庫(kù)，一種情況就是加載ID列表之后，再在數(shù)據(jù)庫(kù)中逐個(gè)查詢ID對(duì)應(yīng)的每條數(shù)據(jù)。
不必要的網(wǎng)絡(luò)調(diào)用：就像數(shù)據(jù)庫(kù)一樣，遠(yuǎn)程服務(wù)有時(shí)也會(huì)被過度使用，原來只要一個(gè)遠(yuǎn)程調(diào)用就可實(shí)現(xiàn)的，或者可以使用批量或緩存防止昂貴網(wǎng)絡(luò)調(diào)用的，卻使用多個(gè)遠(yuǎn)程調(diào)用來實(shí)現(xiàn)。再次強(qiáng)調(diào)，像數(shù)據(jù)庫(kù)一樣，有時(shí)抽象類會(huì)隱藏調(diào)用遠(yuǎn)程API的方法。
移動(dòng)或可穿戴應(yīng)用過于頻繁地調(diào)用后端程序：這基本上和“不必要的網(wǎng)絡(luò)調(diào)用”相同，但是在移動(dòng)設(shè)備上會(huì)產(chǎn)生其他問題，這不僅會(huì)產(chǎn)生不必要的調(diào)用后端使得性能變差，還會(huì)更快地消耗電量甚至導(dǎo)致用戶的金錢支出。
有效且高效地使用資源
代碼是否用鎖來控制共享資源的訪問？這是否會(huì)導(dǎo)致性能降低或死鎖？
鎖是一個(gè)性能開銷大戶，并在多線程環(huán)境中很難理清?？紤]使用以下模式：?jiǎn)尉€程寫或修改值，其余線程只讀，或使用無鎖算法。
是否存在內(nèi)存泄露？Java中一些常見的原因會(huì)是：可變的靜態(tài)字段，使用ThreadLocal變量和使用類加載器。
是否存在內(nèi)存無限增長(zhǎng)？這個(gè)和內(nèi)存泄露不同，內(nèi)存泄漏是指無用的對(duì)象不能被垃圾回收器回收。但對(duì)于任何語言，就算是沒有垃圾回收的語言，也能創(chuàng)建無限變大的數(shù)據(jù)結(jié)構(gòu)。作為審查者，如果你看見新的變量不斷被加到list或map中，你就要問下，這個(gè)list或map什么時(shí)候失效或清除無用數(shù)據(jù)。
代碼是否關(guān)閉了連接或數(shù)據(jù)流？關(guān)閉連接或文件、網(wǎng)絡(luò)數(shù)據(jù)流很容易會(huì)被忘記。當(dāng)你審查別人代碼時(shí)，如果使用到文件、網(wǎng)絡(luò)或數(shù)據(jù)庫(kù)連接，就要確保它們被正確地關(guān)閉了。
資源池是否配置正確？針對(duì)一個(gè)環(huán)境的最佳配置取決于很多因素，所以作為審查者你很難馬上知道像數(shù)據(jù)庫(kù)連接池大小是否正確等這些問題。但是有一些是你一眼就可以看出來的，像資源池是否太?。ū热绱笮≡O(shè)置為1）或太大（如數(shù)百萬線程）。如果無法確定，就從默認(rèn)值開始。沒有使用默認(rèn)值的就需要提供一定的測(cè)試或計(jì)算來證明這么配置的合理性。
審查者可以輕松找出的警告信號(hào)：
一些代碼一眼就能看出存在潛在性能問題。這依賴于所使用的語言和類庫(kù)。
- 反射，Java的反射比正常調(diào)用要慢。如果你在審查含有反射的代碼，你就要問下是否必須使用它。
- 超時(shí)，當(dāng)你審查代碼時(shí)，你可能不知道一個(gè)操作合適的超時(shí)時(shí)間，但是你要想一下“如果超時(shí)了，會(huì)對(duì)系統(tǒng)其他部分造成什么影響？”。作為審查者你應(yīng)該考慮最壞的情況：當(dāng)發(fā)生5分鐘的延時(shí)，應(yīng)用是否會(huì)阻塞？如果超時(shí)時(shí)間設(shè)置成1秒鐘最壞的情況會(huì)是怎么樣的？如果代碼作者不能確定超時(shí)長(zhǎng)度，你作為審查者也不知道一個(gè)選定的時(shí)間的好壞，那么是時(shí)候找一個(gè)理解這其中影響的人參與代碼審查了。
- 并行，代碼是否使用多線程來運(yùn)行一個(gè)簡(jiǎn)單的操作？這樣是否花費(fèi)了更多的時(shí)間以及復(fù)雜度而并沒有提升性能？如果使用現(xiàn)代化的Java，那其中潛在的問題相較于顯示創(chuàng)建線程中的問題更不容易被發(fā)現(xiàn)：代碼是否使用Java 8新的并行流計(jì)算但并沒有從并行中獲益？比如，在少量元素上使用并行流計(jì)算，或者只是運(yùn)行非常簡(jiǎn)單的操作，這可能比在順序流上運(yùn)算還要慢。
正確性
這些不一定影響系統(tǒng)的性能，但是它們與多線程環(huán)境運(yùn)行關(guān)系密切，所以和這個(gè)主題有關(guān)：
代碼是否使用了正確的適合多線程的數(shù)據(jù)結(jié)構(gòu)。
代碼是否存在競(jìng)態(tài)條件（race conditions）？多線程環(huán)境中代碼非常容易造成不明顯的競(jìng)態(tài)條件。作為審查者，可以查看不是原子操作的get和set。
代碼是否正確使用鎖？和競(jìng)態(tài)條件相關(guān)，作為審查者你應(yīng)該檢查被審代碼是否允許多個(gè)線程修改變量導(dǎo)致程序崩潰。代碼可能需要同步、鎖、原子變量來對(duì)代碼塊進(jìn)行控制。
代碼的性能測(cè)試是否有價(jià)值？很容易將小型的性能測(cè)試代碼寫得很糟糕，或者使用不能代表生產(chǎn)環(huán)境數(shù)據(jù)的測(cè)試數(shù)據(jù)，這樣只會(huì)得到錯(cuò)誤的結(jié)果。
緩存：雖然緩存是一種能防止過多高消耗請(qǐng)求的方式，但其本身也存在一些挑戰(zhàn)。如果審查的代碼使用了緩存，你應(yīng)該關(guān)注一些常見的問題，如，不正確的緩存失效方式。
代碼級(jí)優(yōu)化
對(duì)大部分并不是要構(gòu)建低延時(shí)應(yīng)用的機(jī)構(gòu)來說，代碼級(jí)優(yōu)化往往是過早優(yōu)化，所以首先要知道代碼級(jí)優(yōu)化是否必要。
代碼是否在不需要的地方使用同步或鎖操作？如果代碼始終運(yùn)行在單線程中，鎖往往是不必要的。
代碼是否可以使用原子變量替代鎖或同步操作？
代碼是否使用了不必要的線程安全的數(shù)據(jù)結(jié)構(gòu)？比如是否可以使用ArrayList替代Vector？
代碼是否在通用的操作中使用了低性能的數(shù)據(jù)結(jié)構(gòu)？如在經(jīng)常需要查找某個(gè)特定元素的地方使用鏈表。
代碼是否可以使用懶加載并從中獲得性能提升？
條件判斷語句或其他邏輯是否可以將最高效的求值語句放在前面來使其他語句短路？
代碼是否存在許多字符串格式化？是否有方法可以使之更高效？
日志語句是否使用了字符串格式化？是否先使用條件判斷語句校驗(yàn)了日志等級(jí)，或使用延遲求值？
簡(jiǎn)單的代碼即高效的代碼
Java代碼中有一些簡(jiǎn)單的東西可以供審查者尋找，這些會(huì)使JVM很好地替你優(yōu)化你的代碼：
短小的方法和類。
簡(jiǎn)單的邏輯，即消除嵌套的條件或循環(huán)語句。
你在構(gòu)建一個(gè)安全、穩(wěn)固的系統(tǒng)所花費(fèi)的精力，和花在其他特性上的一樣，取決于項(xiàng)目本身，項(xiàng)目運(yùn)行的地方、它使用的用戶、需要訪問的數(shù)據(jù)等。我們現(xiàn)在著重看一些你可能在代碼審查時(shí)關(guān)注的地方。
盡可能使用自動(dòng)化
有驚人數(shù)量的安全檢查可以被自動(dòng)化，而不需要人工干預(yù)。安全測(cè)試不一定要啟動(dòng)所有系統(tǒng)進(jìn)行完整的滲透測(cè)試，一些問題可以在代碼級(jí)就能被發(fā)現(xiàn)。
常見問題如SQL注入或跨站腳本可以在持續(xù)集成環(huán)境通過相應(yīng)工具檢查出。你也能通過OWASP依賴檢測(cè)工具自動(dòng)化檢查你依賴中已知的漏洞。
但是有時(shí)也需要“看情況”，對(duì)有的校驗(yàn)?zāi)憧梢院?jiǎn)單回答“是”或“否”，有時(shí)你需要一個(gè)工具指出潛在的問題，之后再由人工來決定這個(gè)是否需要解決。這也正是Upsource真正的閃光點(diǎn)。Upsource顯示代碼檢查結(jié)果，審查者可以利用這些來決定代碼是否需要改動(dòng)或還可以接受目前的情況。
理解你用到的依賴
第三方類庫(kù)是侵蝕系統(tǒng)安全并引起漏洞的一個(gè)途徑。當(dāng)審查代碼時(shí)至少你要檢查是否引入了新的依賴（如第三方類庫(kù)）。如果你還沒有自動(dòng)化檢查漏洞，你應(yīng)該檢查新引入的類庫(kù)中已知的問題。
你也應(yīng)該嘗試著最小化每個(gè)類庫(kù)的版本，當(dāng)然如果其他依賴有一個(gè)額外的間接依賴，這點(diǎn)可能達(dá)不到。但最簡(jiǎn)單的最小化自己代碼暴露在他人代碼的（通過類庫(kù)或服務(wù)）安全問題中的方法有：
盡可能使用源碼并理解它的可信度。
使用你所能得到的質(zhì)量最高的類庫(kù)。
追蹤你在何處使用了什么，當(dāng)新的漏洞出現(xiàn)，你可以查看你受影響的程度。
檢查是否新的路徑和服務(wù)需要認(rèn)證
無論你開發(fā)web應(yīng)用、提供web服務(wù)或一些其他需要認(rèn)證的API，當(dāng)你增加一個(gè)新的URI或服務(wù)時(shí)，你應(yīng)該確保它不能在沒有認(rèn)證的情況下被訪問（假設(shè)認(rèn)證是你系統(tǒng)的需求）。你只要簡(jiǎn)單地檢查代碼的開發(fā)者寫了合適的測(cè)試用例來展示進(jìn)行了認(rèn)證。
你應(yīng)該不只針對(duì)使用用戶名和密碼的人類用戶來考慮認(rèn)證。其他系統(tǒng)或自動(dòng)化流程來訪問你的應(yīng)用或服務(wù)也會(huì)需要認(rèn)證。這可能影響你們系統(tǒng)中對(duì)“用戶”的定義。
數(shù)據(jù)是否需要加密
當(dāng)你保存一些數(shù)據(jù)到磁盤或通過線纜傳輸，你需要了解數(shù)據(jù)是否應(yīng)該被加密。顯然密碼永遠(yuǎn)不應(yīng)該是簡(jiǎn)單文本，但是有諸多其他情況數(shù)據(jù)需要加密。如果被審查的代碼通過線纜來傳送數(shù)據(jù)或保存在某地或以其他方式離開你的系統(tǒng)，且你不知道它是否應(yīng)該被加密，嘗試詢問下你組織中可以回答這個(gè)問題的人。
密碼是否被很好地控制？
這里的密碼包含密碼（如用戶密碼、數(shù)據(jù)庫(kù)密碼或其他系統(tǒng)的密碼）、秘鑰、令牌等等。這些永遠(yuǎn)不應(yīng)該存放在會(huì)提交到源碼控制系統(tǒng)的代碼或配置文件中，有其他方式管理這些密碼，例如通過密碼服務(wù)器（secret server）。當(dāng)審查代碼時(shí)，要確保這些密碼不會(huì)悄悄進(jìn)入你的版本控制系統(tǒng)中。
代碼的運(yùn)行是否應(yīng)該被日志記錄或監(jiān)控？是否正確地使用？
日志和監(jiān)控需求因各個(gè)項(xiàng)目而不同，一些需要合規(guī)，一些擁有比別人嚴(yán)格的行為、事件日志規(guī)范。如果你有規(guī)章規(guī)定哪些需要記錄日志，何時(shí)、如何記錄，那么作為代碼審查者你應(yīng)該檢查提交的代碼是否滿足要求。如果你沒有固定的規(guī)章，那么就考慮：
代碼是否改變了數(shù)據(jù)（如增刪改操作）？是否應(yīng)該記錄由誰何時(shí)改變了什么？
代碼是否涉及關(guān)鍵性能的部分？是否應(yīng)該在性能監(jiān)控系統(tǒng)中記錄開始時(shí)間和結(jié)束時(shí)間？
每條日志的日志等級(jí)是否恰當(dāng)？一個(gè)好的經(jīng)驗(yàn)法則是“ERROR”觸發(fā)一個(gè)提示發(fā)送到某處，如果你不想這些消息在凌晨3點(diǎn)叫醒誰，那么就將之降級(jí)為“INFO”或“DEBUG”。當(dāng)在循環(huán)中或一條數(shù)據(jù)可能產(chǎn)生多條輸出的情況下，一般不需要將它們記錄到生產(chǎn)日志文件中，它們更應(yīng)該被放在“DEBUG”級(jí)別。
記得叫上專家
安全是個(gè)很大的話題，大到足以讓你的公司聘請(qǐng)技術(shù)安全專家。我們有安全專家就可以獲得他們的幫助，如，邀請(qǐng)他們參加代碼審查，或邀請(qǐng)他們?cè)趯彶榇a時(shí)和我們結(jié)對(duì)。如果這個(gè)無法實(shí)現(xiàn)，我們可以充分學(xué)習(xí)我們系統(tǒng)的環(huán)境，來理解我們有哪種安全需求（面向內(nèi)部的企業(yè)級(jí)應(yīng)用和面向客戶的網(wǎng)頁應(yīng)用有不同的標(biāo)準(zhǔn)），所以我們可以更好地理解我們應(yīng)該在代碼審查中看什么。
代碼審查是一個(gè)很好的方式，不僅確保了代碼質(zhì)量和一致性，也在團(tuán)隊(duì)中或團(tuán)隊(duì)間分享了項(xiàng)目知識(shí)。即使你已經(jīng)自動(dòng)化了基礎(chǔ)的校驗(yàn)，還有許多不同代碼、設(shè)計(jì)的方面需要考慮。代碼審查工具，如Upsource，通過在每個(gè)代碼提交的檢查中高亮可疑的代碼并分析哪些問題已經(jīng)被修復(fù)，新引入哪些問題，可以幫你定位一些潛在的問題。工具也可以簡(jiǎn)化流程，因?yàn)樗峁┝艘粋€(gè)平臺(tái)來討論設(shè)計(jì)和代碼實(shí)現(xiàn)，也可以邀請(qǐng)審查者、作者和其他相關(guān)人員參加討論直到達(dá)成共識(shí)。
最后，團(tuán)隊(duì)需要花時(shí)間決定代碼質(zhì)量的哪些因素對(duì)他們是重要的，也需要專家人工決定哪些規(guī)則應(yīng)用到各個(gè)代碼審查中，參與到審查中的每個(gè)人都應(yīng)該具備并使用人際交往的技巧，如積極的反饋、談判妥協(xié)以達(dá)到最終的共識(shí)，即代碼應(yīng)該怎么樣才“足夠好”可以通過審查。
