-
Notifications
You must be signed in to change notification settings - Fork 1
/
15287284144985.html
542 lines (330 loc) · 31.4 KB
/
15287284144985.html
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
<!doctype html>
<html class="no-js" lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>
Code Review Best Practices - Junkman
</title>
<link href="atom.xml" rel="alternate" title="Junkman" type="application/atom+xml">
<link rel="stylesheet" href="asset/css/foundation.min.css" />
<link rel="stylesheet" href="asset/css/docs.css" />
<script src="asset/js/vendor/modernizr.js"></script>
<script src="asset/js/vendor/jquery.js"></script>
<script src="asset/highlightjs/highlight.pack.js"></script>
<link href="asset/highlightjs/styles/github.css" media="screen, projection" rel="stylesheet" type="text/css">
<script>hljs.initHighlightingOnLoad();</script>
<script type="text/javascript">
function before_search(){
var searchVal = 'site:panlw.github.io ' + document.getElementById('search_input').value;
document.getElementById('search_q').value = searchVal;
return true;
}
</script>
</head>
<body class="antialiased hide-extras">
<div class="marketing off-canvas-wrap" data-offcanvas>
<div class="inner-wrap">
<nav class="top-bar docs-bar hide-for-small" data-topbar>
<section class="top-bar-section">
<div class="row">
<div style="position: relative;width:100%;"><div style="position: absolute; width:100%;">
<ul id="main-menu" class="left">
<li id=""><a target="self" href="index.html">Home</a></li>
<li id=""><a target="_self" href="archives.html">Archives</a></li>
</ul>
<ul class="right" id="search-wrap">
<li>
<form target="_blank" onsubmit="return before_search();" action="http://google.com/search" method="get">
<input type="hidden" id="search_q" name="q" value="" />
<input tabindex="1" type="search" id="search_input" placeholder="Search"/>
</form>
</li>
</ul>
</div></div>
</div>
</section>
</nav>
<nav class="tab-bar show-for-small">
<a href="javascript:void(0)" class="left-off-canvas-toggle menu-icon">
<span> Junkman</span>
</a>
</nav>
<aside class="left-off-canvas-menu">
<ul class="off-canvas-list">
<li><a href="index.html">HOME</a></li>
<li><a href="archives.html">Archives</a></li>
<li><a href="about.html">ABOUT</a></li>
<li><label>Categories</label></li>
<li><a href="Infra.html">Infra</a></li>
<li><a href="Coding.html">Coding</a></li>
<li><a href="Modeling.html">Modeling</a></li>
<li><a href="Archtecting.html">Archtecting</a></li>
</ul>
</aside>
<a class="exit-off-canvas" href="#"></a>
<section id="main-content" role="main" class="scroll-container">
<script type="text/javascript">
$(function(){
$('#menu_item_index').addClass('is_active');
});
</script>
<div class="row">
<div class="large-8 medium-8 columns">
<div class="markdown-body article-wrap">
<div class="article">
<h1>Code Review Best Practices</h1>
<div class="read-more clearfix">
<span class="date">2018/6/11</span>
<span>posted in </span>
<span class="posted-in"><a href='QA.html'>QA</a></span>
<span class="comments">
</span>
</div>
</div><!-- article -->
<div class="article-content">
<blockquote>
<p><a href="https://medium.com/palantir/code-review-best-practices-19e02780015f">https://medium.com/palantir/code-review-best-practices-19e02780015f</a></p>
</blockquote>
<p><img src="https://cdn-images-1.medium.com/max/800/1*6u51C_pl-UH3X3CbggmCjA.png" alt=""/><br/>
<a href="https://xkcd.com/1513/">XKCD ‘Code Quality’, copied under CC BY-NC 2.5</a></p>
<p>The Internet provides a wealth of material on code reviews: <a href="https://blog.fullstory.com/what-we-learned-from-google-code-reviews-arent-just-for-catching-bugs/">on the effect of code reviews on company culture</a>, <a href="https://www.owasp.org/images/2/2e/OWASP_Code_Review_Guide-V1_1.pdf">on formal security reviews</a>, <a href="https://github.com/thoughtbot/guides/tree/master/code-review">shorter guides</a>, <a href="https://www.codeproject.com/Articles/524235/Codeplusreviewplusguidelines">longer checklists</a>, <a href="http://www.processimpact.com/articles/humanizing_reviews.html">humanized reviews</a>, <a href="https://smartbear.com/learn/code-review/why-review-code/">reasons for doing code reviews in the first place</a>, <a href="https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/">best practices</a>, <a href="https://www.atlassian.com/agile/software-development/code-reviews">more best practices</a>, <a href="https://blog.codinghorror.com/code-reviews-just-do-it/">statistics on code review effectiveness for catching bugs</a>, and <a href="https://blog.fogcreek.com/effective-code-reviews-9-tips-from-a-converted-skeptic/">examples of code reviews gone wrong</a>. Oh, and of course there are <a href="https://books.google.com/books/about/Peer_Reviews_in_Software.html?id=d7BQAAAAMAAJ">books</a>, too. Long story short, this blog post presents Palantir’s take on code reviews. Organizations with deep cultural reluctance to peer reviews may want to consult Karl E. Wiegers’ excellent essay on <a href="http://www.processimpact.com/articles/humanizing_reviews.html">Humanizing Peer Reviews</a> before trying to follow this guide.</p>
<p>This post is copied from the <a href="https://github.com/palantir/gradle-baseline/tree/develop/docs">best practices guides</a> of our Java Code Quality tool chain, <a href="https://github.com/palantir/gradle-baseline">Baseline</a>, and covers the following topics:</p>
<ul>
<li> Why, what, and when to do code reviews</li>
<li> Preparing code for review</li>
<li> Performing code reviews</li>
<li> Code review examples</li>
</ul>
<h3 id="toc_0">Motivation</h3>
<p>We perform code reviews (CRs) in order to improve code quality and benefit from <a href="https://blog.fullstory.com/what-we-learned-from-google-code-reviews-arent-just-for-catching-bugs">positive effects on team and company culture</a>. For example:</p>
<ul>
<li> <strong>Committers are motivated</strong> by the notion of a set of reviewers who will look over the change request: the committer tends to clean up loose ends, consolidate TODOs, and generally improve the commit. Recognition of coding expertise through peers is a source of pride for many programmers.</li>
<li> <strong>Sharing knowledge</strong> helps development teams in several ways:
<ul>
<li>A CR explicitly communicates added/altered/removed functionality to team members who can subsequently build on the work done.</li>
<li>The committer may use a technique or algorithm that reviewers can learn from. More generally, code reviews help <a href="https://blog.fullstory.com/what-we-learned-from-google-code-reviews-arent-just-for-catching-bugs/">raise the quality bar across the organization</a>.</li>
<li>Reviewers may possess knowledge about programming techniques or the code base that can help improve or consolidate the change; for example, someone else may be concurrently working on a similar feature or fix.</li>
<li>Positive interaction and communication strengthens social bonds between team members.</li>
</ul></li>
<li> <strong>Consistency</strong> in a code base makes code easier to read and understand, helps prevent bugs, and facilitates collaboration between regular and migratory developer species.</li>
<li> <strong>Legibility</strong> of code fragments is hard to judge for the author whose brain child it is, and easy to judge for a reviewer who does not have the full context. Legible code is more reusable, bug-free, and future-proof.</li>
<li> <strong>Accidental errors</strong> (e.g., typos) as well as <strong>structural errors</strong> (e.g., dead code, logic or algorithm bugs, performance or architecture concerns) are often much easier to spot for critical reviewers with an outside perspective. Studies have found that even short and informal code reviews have significant <a href="https://blog.codinghorror.com/code-reviews-just-do-it/">impact on code quality and bug frequency</a>.</li>
<li> <strong>Compliance</strong> and regulatory environments often demand reviews. CRs are a great way to avoid common security traps. If your feature or environment has significant security requirements it will benefit from (and probably require) review by your local security curmudgeons (<a href="https://www.owasp.org/index.php/Code_Review_Introduction">OWASP’s guide</a> is a good example of the process).</li>
</ul>
<h3 id="toc_1">What to review</h3>
<p>There is no eternally true answer to this question and each development team should agree on its own approach. Some teams prefer to review every change merged into the main branch, while others will have a “triviality” threshold under which a review is not required. The trade-off is between effective use of engineers’ (both authors’ and reviewers’) time and maintaining code quality. In certain regulatory environments, code review may be required even for trivial changes.</p>
<p>Code reviews are classless: being the most senior person on the team does not imply that your code does not need review. Even if, in the rare case, code is flawless, the review provides an opportunity for mentorship and collaboration, and, minimally, diversifies the understanding of code in the code base.</p>
<h3 id="toc_2">When to review</h3>
<p>Code reviews should happen after automated checks (tests, style, other CI) have completed successfully, but before the code merges to the repository’s mainline branch. We generally don’t perform formal code review of aggregate changes since the last release.</p>
<p>For complex changes that should merge into the mainline branch as a single unit but are too large to fit into one reasonable CR, consider a stacked CR model: Create a primary branch <code>feature/big-feature</code> and a number of secondary branches (e.g., <code>feature/big-feature-api</code>, <code>feature/big-feature-testing</code>, etc.) that each encapsulate a subset of the functionality and that get individually code-reviewed against the <code>feature/big-feature</code> branch. Once all secondary branches are merged into <code>feature/big-feature</code>, create a CR for merging the latter into the main branch.</p>
<h3 id="toc_3">Preparing code for review</h3>
<p>It is the author’s responsibility to submit CRs that are easy to review in order not to waste reviewers’ time and motivation:</p>
<ul>
<li> <strong>Scope and size</strong>. Changes should have a narrow, well-defined, self-contained scope that they cover exhaustively. For example, a change may implement a new feature or fix a bug. Shorter changes are preferred over longer ones. If a CR makes substantive changes to more than ~5 files, or took longer than 1–2 days to write, or would take more than 20 minutes to review, consider splitting it into multiple self-contained CRs. For example, a developer can submit one change that defines the API for a new feature in terms of interfaces and documentation, and a second change that adds implementations for those interfaces.</li>
<li> Only submit <strong>complete, self-reviewed</strong> (by diff), and <strong>self-tested</strong> CRs. In order to save reviewers’ time, test the submitted changes (i.e., run the test suite) and make sure they pass all builds as well as all tests and code quality checks, both locally and on the CI servers, <u>before assigning reviewers</u>.</li>
<li> <strong>Refactoring changes</strong> should not alter behavior; conversely, a behavior-changing changes should avoid refactoring and code formatting changes. There are multiple good reasons for this:
<ul>
<li>Refactoring changes often touch many lines and files and will consequently be <a href="https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/">reviewed with less attention</a>. Unintended behavior changes can leak into the code base without anyone noticing.</li>
<li>Large refactoring changes break cherry-picking, rebasing, and other source control magic. It is very onerous to undo a behavior change that was introduced as part of a repository-wide refactoring commit.</li>
<li>Expensive human review time should be spent on the program logic rather than style, syntax, or formatting debates. We prefer settling those with automated tooling like <a href="http://checkstyle.sourceforge.net/">Checkstyle</a>, <a href="https://github.com/palantir/tslint">TSLint</a>, <a href="https://github.com/palantir/gradle-baseline">Baseline</a>, <a href="https://github.com/prettier/prettier">Prettier</a>, etc.</li>
</ul></li>
</ul>
<h3 id="toc_4">Commit messages</h3>
<p>The following is an example of a good commit message following a <a href="http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html">widely quoted standard</a>:</p>
<pre><code>Capitalized, short (80 chars or less) summary
</code></pre>
<pre><code>More detailed explanatory text, if necessary. Wrap it to about 120 characters or so. In some contexts, the firstline is treated as the subject of an email and the rest of the text as the body. The blank line separating thesummary from the body is critical (unless you omit the body entirely); tools like rebase can get confused if you runthe two together.
</code></pre>
<pre><code>Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matchesup with commit messages generated by commands like git merge and git revert.
</code></pre>
<pre><code>Further paragraphs come after blank lines.
</code></pre>
<pre><code>- Bullet points are okay, too
</code></pre>
<p>Try to describe both what the commit changes and how it does it:</p>
<pre><code>> BAD. Don't do this.Make compile again
</code></pre>
<pre><code>> Good.Add jcsv dependency to fix IntelliJ compilation
</code></pre>
<h3 id="toc_5">Finding reviewers</h3>
<p>It is customary for the committer to propose one or two reviewers who are familiar with the code base. Often, one of the reviewers is the project lead or a senior engineer. Project owners should consider subscribing to their projects in order to get notified of new CRs. Code reviews among more than three parties are often unproductive or even counter-productive since different reviewers may propose contradictory changes. This may indicate fundamental disagreement on the correct implementation and should be resolved outside a code review in a higher-bandwidth forum, for example in person or in a video conference with all involved parties.</p>
<h3 id="toc_6">Performing code reviews</h3>
<p>A code review is a synchronization point among different team members and thus has the potential to block progress. Consequently, code reviews need to be prompt (on the order of hours, not days), and team members and leads need to be aware of the time commitment and prioritize review time accordingly. If you don’t think you can complete a review in time, please let the committer know right away so they can find someone else.</p>
<p>A review should be thorough enough that the reviewer could explain the change at a reasonable level of detail to another developer. This ensures that the details of the code base are known to more than a single person.</p>
<p>As a reviewer, it is your responsibility to enforce coding standards and keep the quality bar up. Reviewing code is more of an art than a science. The only way to learn it is to do it; an experienced reviewer should consider putting other less experienced reviewers on their changes and have them do a review first. Assuming the author has followed the guidelines above (especially with respect to self-review and ensuring the code runs), here’s an list of things a reviewer should pay attention to in a code review:</p>
<h4 id="toc_7">Purpose</h4>
<ul>
<li> <strong>Does this code accomplish the author’s purpose?</strong> Every change should have a specific reason (new feature, refactor, bugfix, etc). Does the submitted code actually accomplish this purpose?</li>
<li> <strong>Ask questions.</strong> Functions and classes should exist for a reason. When the reason is not clear to the reviewer, this may be an indication that the code needs to be rewritten or supported with comments or tests.</li>
</ul>
<h4 id="toc_8">Implementation</h4>
<ul>
<li> <strong>Think about how you would have solved the problem.</strong> If it’s different, why is that? Does your code handle more (edge) cases? Is it shorter/easier/cleaner/faster/safer yet functionally equivalent? Is there some underlying pattern you spotted that isn’t captured by the current code?</li>
<li> <strong>Do you see potential for useful abstractions?</strong> Partially duplicated code often indicates that a more abstract or general piece of functionality can be extracted and then reused in different contexts.</li>
<li> <strong>Think like an adversary, but be nice about it.</strong> Try to “catch” authors taking shortcuts or missing cases by coming up with problematic configurations/input data that breaks their code.</li>
<li> <strong>Think about libraries or existing product code.</strong> When someone re-implements existing functionality, more often than not it’s simply because they don’t know it already exists. Sometimes, code or functionality is duplicated on purpose, e.g., in order to avoid dependencies. In such cases, a code comment can clarify the intent. Is the introduced functionality already provided by an existing library?</li>
<li> <strong>Does the change follow standard patterns?</strong> Established code bases often exhibit patterns around naming conventions, program logic decomposition, data type definitions, etc. It is usually desirable that changes are implemented in accordance with existing patterns.</li>
<li> <strong>Does the change add compile-time or run-time dependencies (especially between sub-projects)?</strong> We want to keep our products loosely coupled, with as few dependencies as possible. Changes to dependencies and the build system should be scrutinized heavily.</li>
</ul>
<h4 id="toc_9">Legibility and style</h4>
<ul>
<li> <strong>Think about your reading experience.</strong> Did you grasp the concepts in a reasonable amount of time? Was the flow sane and were variable and methods names easy to follow? Were you able to keep track through multiple files or functions? Were you put off by inconsistent naming?</li>
<li> <strong>Does the code adhere to coding guidelines and code style?</strong> Is the code consistent with the project in terms of style, API conventions, etc.? As mentioned above, we prefer to settle style debates with automated tooling.</li>
<li> <strong>Does this code have TODOs?</strong> TODOs just pile up in code, and become stale over time. Have the author submit a ticket on GitHub Issues or JIRA and attach the issue number to the TODO. The proposed code change should not contain commented-out code.</li>
</ul>
<h4 id="toc_10">Maintainability</h4>
<ul>
<li> <strong>Read the tests.</strong> If there are no tests and there should be, ask the author to write some. Truly untestable features are rare, while untested implementations of features are unfortunately common. Check the tests themselves: are they covering interesting cases? Are they readable? Does the CR lower overall test coverage? Think of ways this code could break. Style standards for tests are often different than core code, but still important.</li>
<li> <strong>Does this CR introduce the risk of breaking test code, staging stacks, or integrations tests?</strong> These are often not checked as part of the pre-commit/merge checks, but having them go down is painful for everyone. Specific things to look for are: removal of test utilities or modes, changes in configuration, and changes in artifact layout/structure.</li>
<li> <strong>Does this change break backward compatibility?</strong> If so, is it OK to merge the change at this point or should it be pushed into a later release? Breaks can include database or schema changes, public API changes, user workflow changes, etc.</li>
<li> <strong>Does this code need integration tests?</strong> Sometimes, code can’t be adequately tested with unit tests alone, especially if the code interacts with outside systems or configuration.</li>
<li> <strong>Leave feedback on code-level documentation, comments, and commit messages.</strong> Redundant comments clutter the code, and terse commit messages mystify future contributors. This isn’t always applicable, but quality comments and commit messages will pay for themselves down the line. (Think of a time you saw an excellent, or truly terrible, commit message or comment.)</li>
<li> <strong>Was the external documentation updated?</strong> If your project maintains a README, CHANGELOG, or other documentation, was it updated to reflect the changes? Outdated documentation can be more confusing than none, and it will be more costly to fix it in the future than to update it now.</li>
</ul>
<p>Don’t forget to praise concise/readable/efficient/elegant code. Conversely, declining or disapproving a CR is not rude. If the change is redundant or irrelevant, decline it with an explanation. If you consider it unacceptable due to one or more fatal flaws, disapprove it, again with an explanation. Sometimes the right outcome of a CR is “let’s do this a totally different way” or even “let’s not do this at all.”</p>
<p>Be respectful to the reviewees. While adversarial thinking is handy, it’s not your feature and you can’t make all the decisions. If you can’t come to an agreement with your reviewee with the code as is, switch to real-time communication or seek a third opinion.</p>
<h4 id="toc_11">Security</h4>
<p>Verify that API endpoints perform appropriate authorization and authentication consistent with the rest of the code base. Check for other common weaknesses, e.g., weak configuration, malicious user input, missing log events, etc. When in doubt, refer the CR to an application security expert.</p>
<h4 id="toc_12">Comments: concise, friendly, actionable</h4>
<p>Reviews should be concise and written in neutral language. Critique the code, not the author. When something is unclear, ask for clarification rather than assuming ignorance. Avoid possessive pronouns, in particular in conjunction with evaluations: “_my_ code worked before <u>your</u> change”, “_your_ method has a bug”, etc. Avoid absolute judgements: “this can <u>never</u> work”, “the result is <u>always</u> wrong”.</p>
<p>Try to differentiate between suggestions (e.g., “Suggestion: extract method to improve legibility”), required changes (e.g., “Add @Override”), and points that need discussion or clarification (e.g., “Is this really the correct behavior? If so, please add a comment explaining the logic.”). Consider providing links or pointers to in-depth explanations of a problem.</p>
<p>When you’re done with a code review, indicate to what extent you expect the author to respond to your comments and whether you would like to re-review the CR after the changes have been implemented (e.g., “Feel free to merge after responding to the few minor suggestions” vs. “Please consider my suggestions and let me know when I can take another look.”).</p>
<h3 id="toc_13">Responding to reviews</h3>
<p>Part of the purpose of the code review is improve the author’s change request; consequently, don’t be offended by your reviewer’s suggestions and take them seriously even if you don’t agree. Respond to every comment, even if it’s only a simple “ACK” or “done.” Explain why you made certain decisions, why some function exists, etc. If you can’t come to an agreement with the reviewer, switch to real-time communication or seek an outside opinion.</p>
<p>Fixes should be pushed to the same branch, but in a separate commit. Squashing commits during the review process makes it hard for the reviewer to follow up on changes.</p>
<p>Different teams have different merge policies: some teams allow only project owners to merge, while other teams allow the contributor to merge after a positive code review.</p>
<h4 id="toc_14">In-person code reviews</h4>
<p>For the majority of code reviews, asynchronous diff-based tools such as Reviewable, Gerrit or, GitHub are a great choice. Complex changes, or reviews between parties with very different expertise or experience can be more efficient when performed in person, either in front of the same screen or projector, or remotely via VTC or screen share tools.</p>
<h3 id="toc_15">Examples</h3>
<p>In the following examples, suggested review comments are indicated by <code>//R: ...</code> comments in the code blocks.</p>
<h4 id="toc_16">Inconsistent naming</h4>
<pre>class MyClass {
private int countTotalPageVisits; //R: name variables consistently
private int uniqueUsersCount;
}</pre>
<h4 id="toc_17">Inconsistent method signatures</h4>
<pre>interface MyInterface {
/** Returns {@link Optional#empty} if s cannot be extracted. */
public Optional<String> extractString(String s);</pre>
<pre> /** Returns null if {@code s} cannot be rewritten. */
//R: should harmonize return values: use Optional<> here, too
public String rewriteString(String s);
}</pre>
<h4 id="toc_18">Library use</h4>
<pre>//R: remove and replace by Guava's MapJoiner
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);</pre>
<h4 id="toc_19">Personal taste</h4>
<pre>int dayCount; //R: nit: I usually prefer numFoo over fooCount; up to you, but we should keep it consistent in this project</pre>
<h4 id="toc_20">Bugs</h4>
<pre>//R: This performs numIterations+1 iterations, is that intentional?
// If it is, consider changing the numIterations semantics?
for (int i = 0; i <= numIterations; ++i) {
...
}</pre>
<h4 id="toc_21">Architectural concerns</h4>
<pre>otherService.call(); //R: I think we should avoid the dependency on OtherService. Can we discuss this in person?</pre>
</div>
<div class="row">
<div class="large-6 columns">
<p class="text-left" style="padding:15px 0px;">
<a href="15287285970632.html"
title="Previous Post: Showdown: MySQL 8 vs PostgreSQL 10">« Showdown: MySQL 8 vs PostgreSQL 10</a>
</p>
</div>
<div class="large-6 columns">
<p class="text-right" style="padding:15px 0px;">
<a href="15283867800691.html"
title="Next Post: 京东多端统一开发框架 - Taro（泰罗）">京东多端统一开发框架 - Taro（泰罗） »</a>
</p>
</div>
</div>
<div class="comments-wrap">
<div class="share-comments">
<script type="text/javascript" src="//s7.addthis.com/js/300/addthis_widget.js#pubid=ra-5ae58078c0d7b2ab"></script>
</div>
</div>
</div><!-- article-wrap -->
</div><!-- large 8 -->
<div class="large-4 medium-4 columns">
<div class="hide-for-small">
<div id="sidebar" class="sidebar">
<div id="site-info" class="site-info">
<div class="site-a-logo"><img src="./asset/img/logo.jpg" /></div>
<h1>Junkman</h1>
<div class="site-des">“拾荒者”一词来自凯文・凯利的《失控》中关于机器学习的故事（“收集癖好机”如何完成他的收集工作）。</div>
<div class="social">
<a target="_blank" class="github" target="_blank" href="https://github.com/panlw/" title="GitHub">GitHub</a>
<a target="_blank" class="rss" href="atom.xml" title="RSS">RSS</a>
</div>
</div>
<div id="site-categories" class="side-item ">
<div class="side-header">
<h2>Categories</h2>
</div>
<div class="side-content">
<p class="cat-list">
<a href="Infra.html"><strong>Infra</strong></a>
<a href="Coding.html"><strong>Coding</strong></a>
<a href="Modeling.html"><strong>Modeling</strong></a>
<a href="Archtecting.html"><strong>Archtecting</strong></a>
</p>
</div>
</div>
<div id="site-categories" class="side-item">
<div class="side-header">
<h2>Recent Posts</h2>
</div>
<div class="side-content">
<ul class="posts-list">
<li class="post">
<a href="15517999043443.html">The Art of Crafting Architectural Diagrams</a>
</li>
<li class="post">
<a href="15517997955971.html">为什么说我们需要软件架构图？</a>
</li>
<li class="post">
<a href="15516128677869.html">DNS Servers That Offer Privacy and Filtering</a>
</li>
<li class="post">
<a href="15516123108194.html">Airbnb's Migration from Monolith to Services</a>
</li>
<li class="post">
<a href="15516097487470.html">Events As First-Class Citizens</a>
</li>
</ul>
</div>
</div>
</div><!-- sidebar -->
</div><!-- hide for small -->
</div><!-- large 4 -->
</div><!-- row -->
<div class="page-bottom clearfix">
<div class="row">
<p class="copyright">Copyright © 2015
Powered by <a target="_blank" href="http://www.mweb.im">MWeb</a>,
Theme used <a target="_blank" href="http://github.com">GitHub CSS</a>.</p>
</div>
</div>
</section>
</div>
</div>
<script src="asset/js/foundation.min.js"></script>
<script>
$(document).foundation();
function fixSidebarHeight(){
var w1 = $('.markdown-body').height();
var w2 = $('#sidebar').height();
if (w1 > w2) { $('#sidebar').height(w1); };
}
$(function(){
fixSidebarHeight();
})
$(window).load(function(){
fixSidebarHeight();
});
</script>
<script src="asset/chart/all-min.js"></script><script type="text/javascript">$(function(){ var mwebii=0; var mwebChartEleId = 'mweb-chart-ele-'; $('pre>code').each(function(){ mwebii++; var eleiid = mwebChartEleId+mwebii; if($(this).hasClass('language-sequence')){ var ele = $(this).addClass('nohighlight').parent(); $('<div id="'+eleiid+'"></div>').insertAfter(ele); ele.hide(); var diagram = Diagram.parse($(this).text()); diagram.drawSVG(eleiid,{theme: 'simple'}); }else if($(this).hasClass('language-flow')){ var ele = $(this).addClass('nohighlight').parent(); $('<div id="'+eleiid+'"></div>').insertAfter(ele); ele.hide(); var diagram = flowchart.parse($(this).text()); diagram.drawSVG(eleiid); } });});</script>
<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.1/MathJax.js?config=TeX-AMS-MML_HTMLorMML"></script><script type="text/x-mathjax-config">MathJax.Hub.Config({TeX: { equationNumbers: { autoNumber: "AMS" } }});</script>
</body>
</html>