TE
科技回声
首页24小时热榜最新最佳问答展示工作
GitHubTwitter
首页

科技回声

基于 Next.js 构建的科技新闻平台,提供全球科技新闻和讨论内容。

GitHubTwitter

首页

首页最新最佳问答展示工作

资源链接

HackerNews API原版 HackerNewsNext.js

© 2025 科技回声. 版权所有。

Code Review Best Practices

221 点作者 Kaedon大约 10 年前

16 条评论

trustfundbaby大约 10 年前
&gt; If the reviewer makes a suggestion, and I don’t have a clear answer as to why the suggestion should not be implemented, I’ll usually make the change<p>This I feel is bad. Code reviews are usually between peers so you shouldn&#x27;t be afraid to seek out clarification where possible. You shouldn&#x27;t be making edits to code that goes in production without clearly understanding why.<p>The other thing that wasn&#x27;t mentioned, that I think is important, is to not act as a blocker for code reviews unless its absolutely necessary. Lots of engineers take on the attitude that they&#x27;re going to &quot;gate&quot; code they don&#x27;t agree with by with holding their +1 and bogging down the review with questions and all sorts of runarounds till its what they want. this is a bad attitude to have, even when you&#x27;re dealing with Junior engineers.<p>I&#x27;m generally going to +1 something unless I fundamentally disagree with it or think its going to break things in production. What I do, though, is leave lots of comments with questions&#x2F;suggestions and mention it in the +1 with (see comments).<p>This builds trust on teams, and stops things getting personal, especially with people who aren&#x27;t very good at dealing with criticism, even in something as banal as a CR. On a team that works well together, teammates will see those comments, think about them and make thoughtful responses, especially once they understand that you&#x27;re not trying to get in their way. Giving the +1 gives them the freedom to consider your suggestions without being irritated that their PR is being blocked. They feel like they&#x27;re in control not you.<p>In rare exceptions, someone will brush off my questions and merge ... which means that next time, I get to be tougher on the review and specifically ask for responses before the code can be merged, because they&#x27;ve degraded the implicit team trust. Usually repeat offenders are assholes, and assholes generally don&#x27;t last on healthy teams.
评论 #9518883 未加载
评论 #9518598 未加载
评论 #9520031 未加载
评论 #9552620 未加载
pablobaz大约 10 年前
While I agree with all the points listed in the article, it highlights for me a major problem of a lot of code reviews.<p>Most code reviews seem to focus on:<p>1. Examining what the change does 2. Finding ways to make the change in a nicer way. E.g. Refactoring etc.<p>This leaves out the key step 0 - what is actually trying to be achieved, does it need to be done and is there a better (maybe completely different) way to do it.<p>This leads to a focus on relatively trivial matters such as naming conventions and method lengths.<p>I think that the underlying reason for this is laziness. Talking about clever refactoring is an easier&#x2F;faster process than understanding the &#x27;why&#x27;.
评论 #9519810 未加载
评论 #9519422 未加载
评论 #9519453 未加载
评论 #9522563 未加载
enqk大约 10 年前
setting thresholds on method &#x2F; class sizes seems quite arbitrary and potentially harmful. Splitting a method into n different ones, none of which is called more than once is setting up the code for opportunistic reuse and obscures it&#x27;s true function. It&#x27;s especially wrong if the code that was split is mutating &#x2F; non pure functional.<p>see <a href="http:&#x2F;&#x2F;number-none.com&#x2F;blow&#x2F;john_carmack_on_inlined_code.html" rel="nofollow">http:&#x2F;&#x2F;number-none.com&#x2F;blow&#x2F;john_carmack_on_inlined_code.htm...</a>
评论 #9518500 未加载
评论 #9519704 未加载
评论 #9518098 未加载
评论 #9518085 未加载
评论 #9519032 未加载
评论 #9519936 未加载
评论 #9518107 未加载
评论 #9518486 未加载
评论 #9518133 未加载
USNetizen大约 10 年前
The one thing that is missing, which ALWAYS seems to fall by the wayside, is security. If people incorporated more iterative security testing (static AND dynamic, automated AND manual) and threat modeling into their SDLC reviews there would be a plummeting number of vulnerabilities.<p>But, because it doesn&#x27;t fit in with the whole &quot;Lean&quot; approach to software (deliver features yesterday), all but the most established enterprises don&#x27;t seem to care much unfortunately. Once more people experience a breach because of their desire to deliver first and remediate vulnerabilities later then perhaps more awareness will be raised. By then it&#x27;s too late though.
maguirre大约 10 年前
I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don&#x27;t know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the &quot;bad things and mistakes&quot; of his code instead of the accomplishments.<p>Has anyone here dealt with similar issues during reviews?
评论 #9518828 未加载
评论 #9519830 未加载
评论 #9518791 未加载
评论 #9518765 未加载
评论 #9519217 未加载
评论 #9518910 未加载
评论 #9518890 未加载
评论 #9518801 未加载
评论 #9518793 未加载
评论 #9519103 未加载
评论 #9519088 未加载
评论 #9519949 未加载
评论 #9518835 未加载
cmpb大约 10 年前
Nice list. This is more or less what I look for. It&#x27;s nice to see your rules of thumb.<p>Anyone have any suggestions for time-estimating code review? That&#x27;s the biggest issue we&#x27;ve faced trying to implement code review into our workflow.
评论 #9518371 未加载
BurningFrog大约 10 年前
This is not a bad list, and can be useful as a checklist.<p>But note that it basically tries to define good programming practice in general. That&#x27;s a very big topic with a lot of room for debate and disagreement.
mikehaggard大约 10 年前
&gt;Variable names: foo or bar are probably not useful method names for data structures. e is similarly not useful when compared to exception. Be as verbose as you need (depending on the language). Expressive variable names make it easier to understand code when we have to revisit it later.<p>I so agree with this!<p>Properly named variables is perhaps THE first line of defense against bad code. Too many developers think they are concise and having little code if they only abbreviate their variable names enough.<p>Honestly, &quot;em&quot;, &quot;erg&quot;, &quot;fc&quot; and &quot;sc&quot; may make perfect sense to use, but it&#x27;s a form of obfuscation to future developers (including your future self).<p>Other pet peeve; adding things to variable names that don&#x27;t add anything meaningful.<p>E.g.<p>&quot;usersList&quot;<p>Does your code really care that it&#x27;s a list? Should the reader be pointed at this each and every time. I much prefer just using:<p>&quot;users&quot;<p>Clear, to the point, and readable.
评论 #9519795 未加载
bottled_poe大约 10 年前
Some of this is frustrating to read. Architectural and detailed design decisions should be made and approved almost entirely before the coding of those features is started. (Obviously this is the ideal and not always possible).<p>This means that the code reviews should involve little more that a checklist of those approved design decisions against their implementation, perhaps a code style is verified as well.<p>Coding without a design is just hacking, which I believe is the primary cause of burn-out and should be avoided as much as possible.<p>So, the question is, do you have a design document? I know it doesn&#x27;t sound very agile, but traditional engineering procedures, when managed well, have a lot of merit in controlling product quality and cost.
shakeel_mohamed大约 10 年前
Another thing to check for that I didn&#x27;t see addressed is debug statements. There&#x27;s nothing quite like seeing console.log(&quot;shit&quot;); during a code review :)
zatkin大约 10 年前
Awesome. I&#x27;m joining Cisco for the summer, so I think this would help me get a head start since they do code review. Thank you!
评论 #9518136 未加载
评论 #9518221 未加载
Too大约 10 年前
This list is very very basic, most of the things like style shouldn&#x27;t have to be discussed and design should preferably be done before the code is written.<p>Just adding &quot;error handling and potential bugs&quot; as a generic bullet on the list just doesn&#x27;t cut it, these should basically be the only items on the list but specified in much greater detail. A serious code review checklist should contain concrete scenarios of these, preferably tailored for your specific application.<p>Examples of this are: What happens during system startup, before all other modules are ready to answer to requests? What happens if the user enters malformed data (sql injections etc)? Does this function shut down gracefully (transaction safe)? How will the algorithms scale? Race conditions and deadlocks? What happens if the network to the database goes down? Is localization handled properly? Backwards compatibility?
justinfreitag大约 10 年前
Style, complexity and coverage checks should be left to automated tooling. Code reviews should focus on whatever remains.
bozoUser大约 10 年前
While I agree with all the points in the blog, I wonder how many programmers do really follow them perfectly(even the author of the blog) because doing code review to such great detail requires plenty of time which is often not the case when you work for corporations.
评论 #9518333 未加载
评论 #9518273 未加载
Kiro大约 10 年前
&gt; If we have to use “and” to finish describing what a method is capable of doing, it might be at the wrong level of abstraction.<p>When I coee, somewhere a method needs to initiate this execution flow and will therefore contain &quot;and&quot;. Even if all it does is call two other methods where this principle is followed. How do I avoid this? I mean, somewhere in the code it makes sense to execute the methods together.
a3voices大约 10 年前
Having worked for businesses that use code reviews and those that don&#x27;t, I personally favor not having code reviews. The reason is that they hinder development speed quite a lot, since you have to try to predict what other engineers will say on your reviews, which takes a lot of brainpower.
评论 #9518414 未加载
评论 #9518675 未加载
评论 #9518512 未加载
评论 #9518783 未加载