Code Review

https://mp.weixin.qq.com/s/YCw7OP3RKF1EJb064IHEXg
一个牛逼的软件，需要有3个重要元素：Reliability, Scalability, Maintainability

在往常经验里，无论是review别人的代码，还是自己提的review，都会有一些共有的问题：

1. code review的工具不一定能够像我们在IDE里面读代码一样支持语义上的跳转去帮助理解，导致review的碎片化，对于reviewer，无法形成一个处理流和整体。
2. 难以对代码涉及到的架构设计、对整个系统的性能影响有全局的思考和审视，review忽视系统，陷入细节。
3. 过多的业务细节和实现细节无法理解。

Code Review很多时候退化成了静态代码分析和自动化测试能够做到的：代码风格检查，局部bug的发现….，比如：函数名/变量名命名较差，更好的算法实现，数据结构实现…代码bug… 。距离在正确达到意图的前提下保证降低复杂度和提高代码可扩展性的目标有一些距离，

一个有经验的工程师都知道，程序设计最耗费脑细胞和时间的可能不是编写代码，而是设计，去在思维的palace里构建出解决问题的模型和思路。对于构建系统来说，也是一样的。 
花费一个团队最多时间的不应该是对test-ready的成品的审查，而是对系统的架构设计、实现/重构方案的讨论与Interface Review。 
在技术驱动的团队里，即使需求很紧急，对于关键的功能，核心成员也会耐心地审视架构设计和实现方案，如何控制熵，如何用更合理的方式实现，如何考虑到未来的变化。 
技术驱动的团队里，应该持续进行对设计的调整和代码的微小重构与改良，时刻在整个系统的设计和表现（performance）角度审视自己的工作。这也是“系统思考”的核心。 
大部分的代码的熵增大难以控制，并不是因为没有好的架构，而是因为迭代中忽略了系统性的思考和审视，而是用局部的解决方案解决问题，在反复迭代后，复杂度过高导致控制熵变得异常困难。这是code review比较难解决的。

除了团队增加对Architecture Design和Interface Review的重视外，保证团队成员的优秀也是非常重要的。一个优秀的工程师需要能够独当一面，能够在系统角度实现局部的良好设计，通过合理的测试方法论验证结果。能够用合理的数据结构、算法实现功能。我想，是一个优秀工程师的基本功。

另外了，大部分的代码风格问题，实现bug问题，性能问题，不一定要通过code review，而是有其他思路的：

1. CI中集成静态代码检查工具，发现潜在的bug和较差的代码风格问题。
2. 工程师具备设计测试case，考虑全面细致的能力。
3. 系统的可操作性，也就是说，具备足够的监控和手段去观察它的健康状态，性能，通过积累的性能分析数据去针对性优化。（切忌过早优化）
4. 自动化集成测试，避免人类的错误（很常见）

另外，Facebook经常发布Bug很多的代码，却并没有产出很多的线上事故的一个重要原因是，具备非常完备的灰度系统和监控设施，在能够精确控制影响范围的前提下，及时发现问题，在更多人被影响之前修复问题。这也是一种很好的思路。

http://letstalkaboutjava.blogspot.com/2016/04/code-review-and-single-responsibility.html

Code Review and the effort

Code review is really useful and important. However, it requires some amount of effort from the reviewer. A developer that will read the code will have to understand what needs to be implemented. They need to know whether expectations are met. They need to have enough understanding to verify the quality of the code and its readability. All of it requires both time and full attention. During the review, if you want to do it right, you have to stay focused.

That’s why we should make code review as small as possible. To decrease the effort that must be put into this activity.

The additional complexity of the review is not the only thing that stands against putting many changes into one commit. What is also important, many changes in one commit mean less readable history so when you want to find something, it would be harder because some changes will be merged with other changes. What would be the name of your commit in that case? It will need to either focus on the most important change or it will be something meaningless, because you will try to use one sentence to explain many things that were modified.

Such thing doesn’t help when you have to find a root cause of some issue or you want to understand some part of the functionality. It is hard because of the same reasons that make the review of such code hard - complexity and presence of many contexts.

One change at a time

What do you think about this idea? 
Let’s organize our commits around a single change. 
Let’s make things simple and easier for us and all of those who will read the code.
https://blog.codeship.com/github-code-review/
One really important part of feature branches and pull requests are proper commit messages.

I personally prefer git-gui for commiting and splitting up commits. 
You have to read through the changes on GitHub at least once before handing the pull request . Making sure no obvious errors are in the code reduces the time and cycles necessary for a good code review.

https://pages.18f.gov/development-guide/code-review/

• expose bugs before they make it to production;
• ensure consistent code quality;
• create an environment for sharing knowledge and developing skills;
• cross-pollinate debugging skills when problems arise;
• cultivate the ability to critique one’s code more strongly;
• encourage open communication between the entire team.

• If commit messages aren't up to par, should they be modified before the PR is

• There is often more than one way to approach a solution. Discuss tradeoffs and reach a resolution quickly.
• Ask questions rather than make statements. ("What do you think about...?")
• Ask for clarification if the code or comments are unclear. ("I didn't understand. Can you please clarify?")
• Avoid selective ownership of code. ("mine", "not mine", "yours")
• Avoid using terms that could be seen as referring to personal traits.
• Be explicit, people don't always understand your intentions online.
• Be humble. ("I'm not sure - let's look it up.")
• Don't use hyperbole. ("always", "never", "endlessly", "nothing")
• Don't use sarcasm.
• Keep it real. If emoji, animated gifs, or humor aren't you, don't force them. If they are, use them with aplomb.
• Talk offline if there is too much back and forth. Post a follow-up comment summarizing the discussion.
• Praise team members when they create exemplary work or suggestions.
• Code reviews require intense concentration, don't forget to factor this in with level of effort estimates.
• Maintaining a well-organized code base requires strict discipline from all team members and will take time and effort to establish; be patient!

• Seek to understand the reviewer's perspective.
• Try to respond to every comment.
• Be grateful for the reviewer's suggestions. ("Good catch, fixing in a4994ec")
• Explain why the code exists. ("We need to work around these existing patterns")
• Extract out-of-scope changes and refactorings into future tasks/issues.
• Push commits based on earlier rounds of feedback as isolated commits to the branch. Do not squash until the branch is ready to merge. Reviewers should be able to read individual updates based on their earlier feedback.

• Understand why the code is necessary (bug, user experience, refactoring)
• Seek to understand the author's perspective.
• Clearly communicate which ideas you feel strongly about and those you don't.
• Identify ways to simplify the code while still solving the problem.
• Offer alternative implementations, but assume the author already considered them. ("What do you think about such-and-such here?")
• Sign off on the pull request with a :thumbsup: or "Ready to merge" comment.
• Wait to merge the branch until it has passed Continuous Integration testing. (TDDium, TravisCI, etc.)

- [ ] Changes are limited to a single goal (no scope creep)
- [ ] Code can be automatically merged (no conflicts)
- [ ] Code follows the standards laid out in this playbook
- [ ] Passes all existing automated tests
- [ ] New functions include new tests
- [ ] New functions are documented (with a description, list of inputs,
      and expected output)
- [ ] Placeholder code is flagged
- [ ] Visually tested in supported browsers and devices
- [ ] Project documentation has been updated (including the "Unreleased"
      section of the CHANGELOG)




- [ ] Do the changes address the project's needs?
- [ ] Do the changes respect the project's existing style?
- [ ] Does the new code avoid reproducing existing functionality?
- [ ] Are functions/classes as simple as possible?
- [ ] Is the code as efficient as possible?
- [ ] Is the usage of each function/class clear?
- [ ] Have edge cases been considered and tested for?
- [ ] Does the code represent a logical unit of work?
- [ ] Are there any glaring syntax errors that were missed?
- [ ] Are language constructs being utilized properly?
- [ ] Are any frameworks/libraries being used leveraged properly?
- [ ] Are there useful comments/documentation where needed?

http://alistapart.com/article/running-code-reviews-with-confidence
Determine the purpose of the proposed
https://medium.com/swlh/code-reviews-can-make-or-break-your-team-a3cfdcc15de1#.svl0s0m0s
4 - Do discuss code smells
5 - Do discuss architecture
6 - Do look for errors that can’t be easily found with tests
https://www.airpair.com/code-review/posts/code-review-the-ultimate-guide

Poor commit messages are very common to find during code reviews. This is too bad, because commits are another way of communicating with other developers through time. When reviewing code, picture yourself reading the commit history one year later. Would you be able to understand it?

Ideally, commits should explain why the changes are being introduced. Developers will be grateful in the future when they look at the commit history in order to understand why the code is the way it is. If the developer can't write a good commit message because there are too many changes to talk about, then you can recommend splitting them into more commits. On the other hand, if there are too many commits with small changes, then squashing them in order to have atomic changes is better.

5.8 Big PRs smell

When reviewing big PRs, try to recognize changes that would be better to digest if they were in their own PR, and recommend that the developer break them into smaller ones. This is usually a sign that the list of tasks described in the story is too long for only one PR.

For instance, suppose the client wants to send reminders of an event and the developer decides to use a solution that schedules background jobs. First, a background solution needs to be put in place — you can recommend that the developer implement this in a separate PR.

5.9 Handling exceptions (unhappy path)

5.10 Third-party dependencies

1. Is the library really necessary to solve the problem?
2. Can we solve the problem on our own?
3. Is it well-tested? Is it battle-tested code running in production? Are there too many issues open?
4. How's the library support? Is it well-known in the community?
5. Is it open source code? Are the maintainers easy to reach out to in case of problems?

This approach relies on the idea that the best person to merge code is the one actively working on it; authors will know better than anyone when they are done.
http://haacked.com/archive/2013/10/28/code-review-like-you-mean-it.aspx/
Keep a code review checklist

1. Ensure there are unit tests and review those first looking for
   test gaps. Unit tests are a fantastic way to grasp how code is
   meant to be used by others and to learn what the expected behavior
   is.
2. Review arguments to methods. Make sure arguments to methods make
   sense and are validated.
3. Look for null reference exceptions. Null references are a
   bitch
   and it’s worth looking out for them specifically.
4. Make sure naming, formatting, etc. follow our conventions and are
   consistent.I like a codebase that’s fairly consistent so you know
   what to expect.
5. Disposable things are disposed. Look for usages of resources
   that should be disposed but are not.
6. Security.

https://wiki.apache.org/hadoop/CodeReviewChecklist

• implementation matches what the documentation says
• logger name is effectively the result of Class.getName()
• class & member access - as restricted as it can be (subject to testing requirements)
• appropriate NullPointerException and IllegalArgumentException argument checks
• asserts - verify they should always be true
• look for accidental propagation of exceptions
• look for unanticipated runtime exceptions
• try-finally used as necessary to restore consistent state
• logging levels conform to Log4j levels
• possible deadlocks - look for inconsistent locking order
• race conditions - look for missing or inadequate synchronization
• consistent synchronization - always locking the same object(s)
• look for synchronization or documentation saying there's no synchronization
• look for possible performance problems
• look at boundary conditions for problems
• configuration entries are retrieved/set via setter/getter methods
• implementation details do NOT leak into interfaces
• variables and arguments should be interfaces where possible
• if equals is overridden then hashCode is overridden (and vice versa)
• objects are checked (instanceof) for appropriate type before casting (use generics if possible)
• public API changes have been publically discussed
• use of static member variables should be used with caution especially in Map/reduce tasks due to the JVM reuse feature

http://kevinlondon.com/2015/05/05/code-review-best-practices.html

• Code duplication: I go by the “three strikes” rule. If code is copied once, it’s usually okay although I don’t like it. If it’s copied again, it should be refactored so that the common functionality is split out.

• Code left in a better state than found: If I’m changing an area of the code that’s messy, it’s tempting to add in a few lines and leave. I recommend going one step further and leaving the code nicer than it was found.
• Potential bugs: Are there off-by-one errors? Will the loops terminate in the way we expect? Will they terminate at all?
• Error handling: Are errors handled gracefully and explicitly where necessary? Have custom errors been added? If so, are they useful?
• Efficiency: If there’s an algorithm in the code, is it using an efficient implementation? For example, iterating over a list of keys in a dictionary is an inefficient way to locate a desired value.

• Function length: My rule of thumb is that a function should be less than 20 or so lines. If I see a method above 50, I feel it’s best that it be cut into smaller pieces.

• Commented code: Good idea to remove any commented out lines.
• Number of method arguments: For the methods and functions, do they have 3 or fewer arguments? Greater than 3 is probably a sign that it could be grouped in a different way.
• Readability: Is the code easy to understand? Do I have to pause frequently during the review to decipher it?

• Number of Mocks: Mocking is great. Mocking everything is not great. I use a rule of thumb where if there’s more than 3 mocks in a test, it should be revisited. Either the test is testing too broadly or the function is too large. Maybe it doesn’t need to be tested at a unit test level and would suffice as an integration test. Either way, it’s something to discuss.
• Meets requirements: Usually as part of the end of a review, I’ll take a look at the requirements of the story, task, or bug which the work was filed against.

Review your own code first

• Did I leave a comment or TODO in?
• Does that variable name make sense?
• …and anything else that I’ve brought up above.

• Ask questions: How does this method work? If this requirement changes, what else would have to change? How could we make this more maintainable?
• Compliment / reinforce good practices

• Discuss in person for more detailed points: On occasion, a recommended architectural change might be large enough that it’s easier to discuss it in person rather than in the comments. Similarly, if I’m discussing a point and it goes back and forth, I’ll often pick it up in person and finish out the discussion.
• Explain reasoning: I find it’s best both to ask if there’s a better alternative and justify why I think it’s worth fixing. Sometimes it can feel like the changes suggested can seem nit-picky without context or explanation.
• Make it about the code: It’s easy to take notes from code reviews personally, especially if we take pride in our work. It’s best, I find, to make discussions about the code than about the developer. It lowers resistance and it’s not about the developer anyway, it’s about improving the quality of the code.
• Suggest importance of fixes

• All the Small Things by Sandi Metz: Covers the topic well, particularly from a perspective of writing clean, reusable code.
• How to Design a Good API and Why it Matters: API, in this sense, meaning the way in which the application is constructed and how we interact with it. Many of the points in the video talk about similar themes to those discussed here.

http://kevinlondon.com/2015/06/25/intro-to-code-reviews-talk.html
http://insights.dice.com/2012/10/30/how-to-do-effective-peer-code-reviews/

• Timeliness counts. On a team of more than five people, no code review request should sit more than half a business day. Peer code reviews can be a real bottleneck if they build up; it’s much better to take them as they come in. And in the end, a code review makes a good thing to do when you need a break from a hard problem or when you’re transitioning between tasks.

http://swreflections.blogspot.com/2014/08/dont-waste-time-on-code-reviews.html

Research backs up what should be obvious: the effectiveness of code reviews depend heavily on the reviewer’s skill and familiarity with the problem domain and with the code. Like other areas in software development, the differences in revew effectiveness can be huge, as much as 10x between best and worst performers. A study on code reviews at Microsoft found that reviewers from outside of the team or who were new to the team and didn’t know the code or the problem area could only do a superficial job of finding formatting issues or simple logic bugs.

This means that your best developers, team leads and technical architects will spend a lot of time reviewing code – and they should. You need reviewers who are good at reading code and good at debugging, and who know the language, framework and problem area well. They will do a much better job of finding problems, and can provide much more valuable feedback, including suggestions on how to solve the problem in a simpler or more efficient way, or how to make better use of the language and frameworks. And they can do all of this much faster.

Free up reviewers to focus on the things that matter: helping developers write better code, code that works correctly and that is easy to maintain.

Correctness – make sure that the code works, look for bugs that might be hard to find in testing:

• Functional correctness: does the code do what it is supposed to do – the reviewer needs to know the problem area, requirements and usually something about this part of the code to be effective at finding functional correctness issues
• Coding errors: low-level coding mistakes like using <= instead of <, off-by-one errors, using the wrong variable (like mixing up lessee and lessor), copy and paste errors, leaving debugging code in by accident
• Design mistakes: errors of omission, incorrect assumptions, messing up architectural and design patterns like MVC, abuse of trust
• Safety and defensiveness: data validation, threading and concurrency (time of check/time of use mistakes, deadlocks and race conditions), error handling and exception handling and other corner cases
• Malicious code: back doors or trap doors, time bombs or logic bombs
• Security: properly enforcing security and privacy controls (authentication, access control, auditing, encryption)

Maintainability:

• Clarity: class and method and variable naming, comments, …
• Consistency: using common routines or language/library features instead of rolling your own, following established conventions and patterns
• Organization: poor structure, duplicate or unused/dead code
• Approach: areas where the reviewer can see a simpler or cleaner or more efficient implementation

Research shows that reviewers find far more maintainability issues than defects (a ratio of 75:25) andspend more time on code clarity and understandability problems than correctness issues. There are a few reasons for this.

Finding bugs in code is hard. Finding bugs in someone else’s code is even harder.

In many cases, reviewers don’t know enough to find material bugs or offer meaningful insight on how to solve problems. Or they don’t have time to do a good job. So they cherry pick easy code clarity issues like poor naming or formatting inconsistencies.

But even experienced and serious reviewers can get caught up in what at first seem to be minor issues about naming or formatting, because they need to understand the code before they can find bugs, and code that is unnecessarily hard to read gets in the way and distracts them from more important issues.

This is why programmers at Microsoft will sometimes ask for 2 different reviews: a superficial “code cleanup” review from one reviewer that looks at standards and code clarity and editing issues, followed by a more in depth review to check correctness after the code has been tidied up.

Do reviews early and often. Keep the feedback loops as tight as possible.

Ask everyone to take reviews seriously – developers and reviewers. No rubber stamping, or letting each other off of the hook.

Make reviews simple, but not sloppy. Ask the reviewers to focus on what really matters: correctness issues, and things that make the code harder to understand and harder to maintain. Don’t waste time arguing about formatting or style.

Make sure that you always review high risk code and high risk changes.

Get the best people available to do the job – when it comes to reviewers, quality is much more important than quantity. Remember that code reviews are only one part of a quality program. Instead of asking more people to review code, you will get more value by putting time into design reviews or writing better testing tools or better tests.

http://mikehadlow.blogspot.com/2009/05/what-i-look-for-in-code-review.html

General Principles

• The core imperative is to organise complexity.
• Clarity and readability is central. “Intention Revealing”
• Do not prematurely optimise for performance.
• Do not repeat yourself. Never copy-and-paste code.
• Decouple.
• Always try to leave the code you work on in a better state than before you started (the ‘boy scout’ principle)

Keep the source clean

• Always delete unused code. Including variables and using statements
• Don’t comment out code, delete it. We have source control to manage change.

Naming things

• The name should accurately describe what the thing does.
• Do not use shortenings, only use well understood abbreviations.
• If the name looks awkward, the code is probably awkward.

Namespaces

• Namespaces should match the project name + path inside the project. This is what VS will give you by default.
• Classes that together provide similar functions should be grouped in a single namespace.
• Avoid namespace dependency cycles.

Variables

• Use constants where possible. Avoid magic strings.
• Use readonly where possible
• Avoid many temporary variables.
• Never use a single variable for two different puposes.
• Keep scope as narrow as possible. (declaration close to use)

Methods

• The name should accurately describe what the method does.
• It should only do one thing.
• It should be small (more than 10 lines of code is questionable).
• The number of parameters should be small.
• Public methods should validate all parameters.
• Assert expectations and throw an appropriate error if invalid.
• Avoid deep nesting of loops and conditionals. (Cyclomatic complexity).

Classes

• The name should accurately describe what the class does.
• Classes typically represent data or services, be clear which your class is.
• Design your object oriented schema deliberately.
• A class should be small.
• A class should have one responsibility only.
• A class should have a clear contract.
• A class should be decoupled from its dependencies.
• Favour composition over inheritance.
• Avoid static classes and methods.
• Make the class immutable if possible.

Interfaces

• Rely on interfaces rather than concrete classes wherever possible.
• An interface is a contract for interaction.
• An interface should have a single purpose (ISP)

Tests

• All code should have unit tests if possible.
• Test code should have the same quality as production code.
• Write code test-first wherever possible.

Error Handling

• Only wrap code with a try..catch statement if you are expecting it to throw a specific exception.
• Unexpected errors should only be handled at process boundaries.
• Never ‘bury’ exceptions.

http://blog.fogcreek.com/effective-code-reviews-9-tips-from-a-converted-skeptic/

• Catch bugs
• Ensure code is readable and maintainable
• Spread knowledge of the code base throughout the team
• Get new people up to speed with the ways of working
• Expose everyone to different approaches

Review the right things, let tools to do the rest

You don’t need to argue over code style and formatting issues. There are plenty of tools which can consistently highlight those things. Ensuring that the code is correct, understandable and maintainable is what’s important.

Everyone should code review
Review all code

Code review often and for short sessions

It’s OK to say “It’s all good”

Use a checklist

Keep the code short

Provide context

http://blogs.msdn.com/b/cdndevs/archive/2014/05/07/what-should-i-be-looking-for-during-code-reviews.aspx

Is there any dead code?

Dead code is just as bad as misleading comments. Don’t keep unused code in your solution because it’s just going to accumulate. Developers will be wary and won’t get rid of it because there’s too much uncertainty. What’s the worst that can happen? The solution will bloat, developers will get lost and productivity will drop off the map.

As professionals, we should strive to use source control for what its best at, keeping historic snapshots of our code. Keeping our code clean, means that we don’t have to ask unnecessary questions and that we can concentrate on real problems.

Was the developer too creative?

The last question I ask myself about the code I’m reviewing, concerns creativity. Was the developer too creative with their proposed solution? Will other developers on our team understand and will they be capable of maintaining the code? If the answer to these questions is “NO”, I spend some time with the developer to simplify the overall solution.

http://www.cio.com/article/2431554/developer/what-to-look-for-in-a-code-review.html
Pay attention to the programming language. "People often make mistakes similar to typos around pointer misuse, missing breaks in a switch, etc."
Take the time to understand the impact of the code. If it is very localized you should be able to review quickly. If it has larger impact you need to take more time.
If you are having a hard time understanding the code, stop and ask for help. Don't waste a lot of time trying to figure it out.

code review concentrate on reusability, easy conversion to other locales, use of standard procedures and functions whenever possible, well defined interfaces, system calls and file formats.

Focus on catching items that the compiler, linter and other automated tools miss

most new code reviewers don't follow it, and time spent on such items is mostly wasted," Brown says. It's far better for code reviewers to look for logic errors, he says; the chief mandate is to find those sorts of glitches where the code doesn't do exactly what it's supposed to (even if it is clean and appears to work in most cases).

Reviewers should hunt down potential problems where working code could break.

Are we doing the same thing multiple times without using a shared function?
"Are we doing the same thing multiple times without using a shared function?"

http://blog.codinghorror.com/code-reviews-just-do-it/

Peer review – an activity in which people other than the author of a software deliverable examine it for defects and improvement opportunities – is one of the most powerful software quality tools available. Peer review methods include inspections, walkthroughs, peer deskchecks, and other similar activities. After experiencing the benefits of peer reviews for nearly fifteen years, I would never work in a team that did not perform them.

After participating in code reviews for a while here at Vertigo, I believe that peer code reviews are the single biggest thing you can do to improve your code. If you're not doing code reviews right now with another developer, you're missing a lot of bugs in your code and cheating yourself out of some key professional development opportunities. As far as I'm concerned, my code isn't done until I've gone over it with a fellow developer.
http://web.mit.edu/6.005/www/fa15/general/code-review.html

Bugs or potential bugs.

• Repetitive code (remember DRY, Don’t Repeat Yourself).
• Disagreement between code and specification.
• Off-by-one errors.
• Global variables, and other too-large variable scopes.
• Optimistic, undefensive programming.
• Magic numbers.
• …

Unclear, messy code.

• Bad variable or method names.
• Inconsistent indentation.
• Convoluted control flow (if and while statements) that could be simplified.
• Packing too much into one line of code, or too much into one method.
• Failing to comment obscure code.
• Having too many trivial comments that are simply redundant with the code.
• Variables used for more than one purpose.
• …

Misunderstandings of Java.

• Misuse of == or .equals().
• Misuse of arrays or Lists.
• …

Misusing (or failing to use) essential design concepts.

• Incomplete or incorrect specification for a method or class.
• Representation exposure for a data abstraction.
• Immutable datatypes that expose themselves to change.
• Invariants that aren’t really invariant, or aren’t even stated.
• Failure to implement the Object contract correctly (equals and hashCode).

http://www.developer.com/tech/article.php/3579756/Effective-Code-Reviews-Without-the-Pain.htm
http://www.processimpact.com/articles/humanizing_reviews.html

Developer

Less time spent performing rework Increased programming productivity Better techniques learned from other developers Reduced unit testing and debugging time Less debugging during integration and system testing Exchanging of information about components and overall system with other team members

Project Manager

Shortened product development cycle time Increased chance of shipping the product on schedule Reduced field service and customer support costs Reduced lifetime maintenance costs, freeing resources for new development projects Improved teamwork, collaboration, and development effectiveness Reduced impact from staff turnover through cross-training of team members Better and earlier insight into project risks and quality issues

http://guides.beanstalkapp.com/code-review/guide-to-code-review.html

• Fewer bugs. Developers can continue making errors as usual, but code reviews will decrease the amount of those bugs that make it to production.
• Better security. Code reviews make it easy to spot potential vulnerabilities and fix them before they make their way to your servers.
• Knowledge sharing. Members of your team can learn from each other by reviewing each other’s code.
• Code quality. Things like readability, efficiency, and maintainability of your code may not always directly affect your users but they are tremendously important in the long run of your project.
• Better performance. Code reviews help spot performance issues and regressions.

https://www.bignerdranch.com/blog/the-4-things-we-look-for-in-a-code-review/
https://msdn.microsoft.com/en-us/library/bb871031.aspx

https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/

9. Embrace the subconscious implications of peer review

The knowledge that others will be examining their work naturally drives people to produce a better product. This "Ego Effect" naturally incentivizes developers to write cleaner code because their peers will certainly see it.
http://coolshell.cn/articles/1302.html

1. Code reviews 不应该承担发现代码错误的职责。Code Review主要是审核代码的质量，如可读性，可维护性，以及程序的逻辑和对需求和设计的实现。代码中的bug和错误应该由单元测试，功能测试，性能测试，回归测试来保证的（其中主要是单元测试，因为那是最接近Bug，也是Bug没有扩散的地方）
2. Code reviews 不应该成为保证代码风格和编码标准的手段。编码风格和代码规范都属于死的东西，每个程序员在把自己的代码提交团队Review的时候，代码就应该是符合规范的，这是默认值，属于每个人自己的事情，不应该交由团队来完成，否则只会浪费大家本来就不够的时间。我个人认为“meeting”是奢侈的，因为那需要大家在同一时刻都挤出时间，所以应该用在最需要的地方。代码规范比起程序的逻辑和对需求设计的实现来说，太不值得让大家都来了。

尽可能的让不同的人Reivew你的代码

这是一个好主意，如果可能的话，不要总是只找一个人来Review你的代码，不同的人有不同的思考方式，有不同的见解，所以，不同的人可以全面的从各个方面评论你的代码，有的从实现的角度，有的从需求的角度，有的从用户使用的角度，有的从算法的角度，有的从性能效率的角度，有的从易读的角度，有的从扩展性的角度

为什么要坚持code review

有和没有之间的态度差别
很简单，一段代码完成之后，有人看和没有人看，在质量上还是会有差别的。
当你知道你的代码会被人一行一行review时，你的代码一定为努力写的最好，而不是为了完成功能而应付了事，这就是态度上的区分。因为当你知道你写的代码是有人看的，你会更加在意你写的东西，你一定不想背后被人说，代码写的像一坨**。
而且确实我自己在review代码时，就能看出哪些为了赶上线而写的就和平常写的会有差别。

代码的可读，可维护
代码的可读和可维护在大团队很关键，最初我们代码审核为什么严格到令人发指，就是因为可读性和可维护性。
严格的code review可以感觉整个团队写出的代码就像是一个人写的。这样就是为了能让你随时切换到其他业务上，而且不需要什么适应时间。
可读性和可维护对于大型的多人合作系统，可以说非常关键，当一个团队30多人在一个单页面开发时，如果风格各异，代码仅仅符合功能和测试要求的话，你会发现多人开发的成本和新需求的升级的成本会越来越高。

举个常见的例子：
如果某个业务突然需要提高进度，这时的办法就是加人力，但是如果代码风格各异，加入的人力适应时间和学习成本是极高的，有时甚至不如保持原有人力去加班hold。要不就只能找些技术很强，可以无障碍学习的资深工程师江湖救急。

我们这边其实就是会出现上面项目突然加急的情况，但是，因为有code review，所以我们多人合作时，基本上可以保证1+1≈2的效率。为什么这么任性，就是因为在code review时已经控制了大家的写法和模式，让新加入的同学能够马上看懂以前逻辑并且做大概的业务了解就能上手了。
我这边最近就经历了这些，因为需要还一些历史遗留的技术债务，所以我需要调整底层的一些代码结构，这时保证功能和互相调用ok的情况下切换代码位置和路径就是我遇到的最大的障碍。
不过由于之前业务代码的高质量和开发模式基本上完全一致，所以我能很快找到统一的切入点，预先就能预估好可能会踩的坑。
如果没有code review之前严格的约束，我基本上需要每个业务功能去分析了，效率降到极低。

对于新人，快速引导，快速反馈
对于新人加入我们的团队，最快的上手方式就是，简单熟悉完之后，接手一个业务项目，然后我们会通过不断的code review给与开发方式，编码习惯，设计模式之间的反馈。
第一个项目的review 基本上会是最严格的。
其实对于技术人员，code review 就是用代码在做沟通。

及时发现非代码上的问题
有时我在review代码时会发现，有些地方经常在反复修改，或者有时实现会非常别扭。这时我就会问开发人员，是不是需求不明确导致的，是不是对需求的理由有偏差。
因为对于正在开发的同学，经常会陷入到业务具体的实现中，有时就是饶了很大的圈子但是自己确不知道。这时review时是能感觉到的，而且我也成功的多次阻止过。

https://codingstyle.cn/topics/36

• tech leader 强压所有人开始 code review, 这是最重要的一步
• 安排一次编码规范的技术分享
• 前期经常回顾, 这次的code review开展的怎样, 有哪些地方可以改善
• 对于积极的同学表示鼓励, 支持现场重构代码
• 每天不光可以review代码, 也可以安排整场的技术分享

https://codingstyle.cn/topics/35

代码回顾的学习重点，是团队成员共同识别模式。这里的模式指的是程序员编写代码的习惯，包括“好模式”和“反模式”。像富有表达力的类名、单一职责的方法、良好的格式缩进等等，都是“好模式”。而像那些令人迷惑的缩写、几百行的一个类文件、复杂的if-else嵌套等等，都是“反模式”。团队成员通过阅读最近编写的测试代码和生产代码，来一起识别“好模式”和“反模式”，既是团队成员之间相互学习的过程，也是团队整体达成编写整洁代码共识的过程。

既然代码回顾的学习重点是识别代码编写的好模式和反模式，那么代码的作者就不是重点。在代码回顾的过程中，完全可以不提谁是代码的作者，而只提“好模式”和“反模式”，这样能让作者放松心态，更好地接受合理的建议。

既然代码回顾的学习重点是识别代码编写的好模式和反模式，那么在代码回顾中发现的bug也不是代码回顾活动的重点。老虎也有打盹儿的时候，谁不犯错儿呢？好模式和反模式，其实就是编程的好习惯和坏习惯。代码回顾应该重在识别编程习惯，而不是找bug。

另外需要注意的是，一些高手在代码回顾时，即使代码本身已经符合整洁代码的要求，他们也会不自觉地提出自己的不同写法，甚至会提出另一种全新的设计。高手们提出这些看法，虽然很有价值，但都不是代码回顾所关注的。高手们可以在代码回顾会后，私下再找作者沟通，这样能令代码回顾会议更专注和高效。

只识别反模式，不识别好模式，会让代码回顾退化到令人生畏的代码审查，打掉大家长期坚持的积极性。

把Code Review称作“代码回顾”吧，而不要称作令人紧张的“代码评审”或“代码走查”，把它打造成软件开发团队“共同学习、识别模式和每日持续”的过程，来有效提升团队代码内在质量。持续”的过程，来有效提升团队代码内在质量。

http://www.openfoundry.org/tw/tech-column/9225-code-review-
Code Review最佳实践

• 单一职责原则.
  • 这是经常被违背的原则。一个类只能干一个事情, 一个方法最好也只干一件事情。 比较常见的违背是一个类既干UI的事情，又干逻辑的事情, 这个在低质量的客户端代码里很常见。
• 行为是否统一
  • 比如缓存是否统一，错误处理是否统一， 错误提示是否统一， 弹出框是否统一 等等。
  • 同一逻辑/同一行为 有没有走同一Code Path？低质量程序的另一个特征是，同一行为/同一逻辑，因为出现在不同的地方或者被不同的方式触发，没有走同一Code Path 或者各处有一份copy的实现， 导致非常难以维护。
• 代码污染
  • 代码有没有对其他模块强耦合 ？
• 重复代码
  • 主要看有没有把公用组件，可复用的代码，函数抽取出来。
• Open/Closed 原则
  • 就是好不好扩展。 Open for extension, closed for modification.
• 面向接口编程 和 不是 面向实现编程
  • 主要就是看有没有进行合适的抽象， 把一些行为抽象为接口。
• 健壮性
  • 有没有考虑线程安全性， 数据访问的一致性
  • 对Corner case有没有考虑完整，逻辑是否健壮？有没有潜在的bug？
  • 有没有内存泄漏？有没有循环依赖?（针对特定语言，比如Objective-C) ？有没有野指针？
• 错误处理
  • 有没有很好的Error Handling？比如网络出错，IO出错。
• 改动是不是对代码的提升
  • 新的改动是打补丁，让代码质量继续恶化，还是对代码质量做了修复？
• 效率/性能
  • 关键算法的时间复杂度多少？有没有可能有潜在的性能瓶颈。
  • 客户端程序 对频繁消息 和较大数据等耗时操作是否处理得当。

其中有一部分问题，比如一些设计原则， 可预见的效率问题， 开发模式一致性的问题 应该尽早在Design Review阶段解决。如果Design阶段没有解决，那至少在Code Review阶段也要把它找出来。

Style

• 可读性
  • 衡量可读性的可以有很好实践的标准，就是Reviewer能否非常容易的理解这个代码。 如果不是，那意味着代码的可读性要进行改进。
• 命名
  • 命名对可读性非常重要，我倾向于函数名/方法名长一点都没关系，必须是能自我阐述的。
  • 英语用词尽量准确一点（哪怕有时候需要借助Google Translate，是值得的）
• 函数长度/类长度
  • 函数太长的不好阅读。 类太长了，比如超过了1000行，那你要看一下是否违反的“单一职责” 原则。
• 注释
  • 恰到好处的注释。 但更多我看到比较差质量的工程的一个特点是缺少注释。
• 参数个数
  • 不要太多， 一般不要超过3个。

Review Your Own Code First

• 跟著名的橡皮鸭调试法（Rubber Duck Debugging）一样，每次提交前整体把自己的代码过一遍非常有帮助，尤其是看看有没有犯低级错误。

如何进行Code Review

• 多问问题。多问 “这块儿是怎么工作的？” “如果有XXX case，你这个怎么处理？”
• 每次提交的代码不要太多，最好不要超过1000行，否则review起来效率会非常低。
• 当面讨论代替Comments。 大部分情况下小组内的同事是坐在一起的，face to face的 code review是非常有效的。
• 区分重点，不要舍本逐末。 优先抓住 设计，可读性，健壮性等重点问题。

Code Review的意识

• 作为一个Developer , 不仅要Deliver working code, 还要Deliver maintainable code.
• 必要时进行重构，随着项目的迭代，在计划新增功能的同时，开发要主动计划重构的工作项。
• 开放的心态，虚心接受大家的Review Comments。

https://segmentfault.com/a/1190000004502976

在很多跟代码质量有关的书里都强调了一个观点：程序首先是给人看的，其次才是能被机器执行，我也比较认同这个观点。在评价一段代码能不能让人看懂的时候，我习惯让作者把这段代码逐字翻译成中文，试着组成句子，之后把中文句子读给另一个人没有看过这段代码的人听，如果另一个人能听懂，那么这段代码的可读性基本就合格了。
用这种判断方式的原因很简单：其他人在理解一段代码的时候就是这么做的。阅读代码的人会一个词一个词的阅读，推断这句话的意思，如果仅靠句子无法理解，那么就需要联系上下文理解这句代码，如果简单的联系上下文也理解不了，可能还要掌握更多其它部分的细节来帮助推断。大部分情况下，理解一句代码在做什么需要联系的上下文越多，意味着代码的质量越差。
逐字翻译的好处是能让作者能轻易的发现那些只有自己知道的、没有体现在代码里的假设和可读性陷阱。无法从字面意义上翻译出原本意思的代码大多都是烂代码，比如“ms代表messageService“，或者“ms.proc()是发消息“，或者“tmp代表当前的文件”。

3.2 Don’t Repeat Yourself
一般对这条原则的理解是对于同样的功能不要直接copy原来的代码，而是要抽象出一个公用的方法。但是实际上对同样的功能用不同的思路或者代码去实现也是一种浪费。比如常见的日志处理、异常处理逻辑。

3.3 Prefer Composition to Inheritance
这条原则跟前面提到的OOP的SOLID原则里面的Interface segregation principle有点重合之处。随着业务需求的不断迭代，小的组件逐渐会演变成大的组件，在这个过程中驾驭的难度会逐步提升，而如果在不断迭代的过程中不断抽象出小的组件，则可以在业务功能复杂的同时保持代码的简洁。比如不管是飞机还是汽车火车都是会移动的，而我在使用时只需知道这个对象是可移动的即可，至于这个对象是飞机还是汽车我并不关心。

其次，要把握review的粒度，不要一下发起一个非常大的PR，这样会给review的同学特别大的压力。比如一个PR里最好不要同时既有重构又有新特性的开发，或者憋到最后这个版本都要开发完了才一起提交一个PR。review应该是在平时的工作中持续进行的，而不是类似里程碑的总结之类的东西。

第三，code review不应该承担发现业务逻辑错误的责任，也就是平常我们所说的bug，bug应该由单元测试、功能测试、性能测试等方法来保证，不要赋予code review太多的责任。