Code Review
  • Written by Liang Zeng
  • September 6th, 2015
  • About review

Code Review

常规项

  • 代码能够工作么?它有没有实现预期的功能,逻辑是否正确等。
  • 所有的代码是否简单易懂?
  • 代码符合你所遵循的编程规范么?这通常包括大括号的位置,变量名和函数名,行的长度,缩进,格式和注释。
  • 是否存在多余的或是重复的代码?
  • 代码是否尽可能的模块化了?
  • 是否有可以被替换的全局变量?
  • 是否有被注释掉的代码?
  • 循环是否设置了长度和正确的终止条件?
  • 是否有可以被库函数替代的代码?
  • 是否有可以删除的日志或调试代码?

安全

  • 所有的数据输入是否都进行了检查(检测正确的类型,长度,格式和范围)并且进行了编码?
  • 在哪里使用了第三方工具,返回的错误是否被捕获?
  • 输出的值是否进行了检查并且编码?
  • 无效的参数值是否能够处理?

文档

  • 是否有注释,并且描述了代码的意图?
  • 所有的函数都有注释吗?
  • 对非常规行为和边界情况处理是否有描述?
  • 第三方库的使用和函数是否有文档?
  • 数据结构和计量单位是否进行了解释?
  • 是否有未完成的代码?如果是的话,是不是应该移除,或者用合适的标记进行标记比如‘TODO’?

测试

  • 代码是否可以测试?比如,不要添加太多的或是隐藏的依赖关系,不能够初始化对象,测试框架可以使用方法等。
  • 是否存在测试,它们是否可以被理解?比如,至少达到你满意的代码覆盖(code coverage)。
  • 单元测试是否真正的测试了代码是否可以完成预期的功能?
  • 是否检查了数组的“越界“错误?
  • 是否有可以被已经存在的API所替代的测试代码?

优化你的清单

把使用清单作为你的起点,针对特定的使用案例,你需要对其进行优化。一个比较棒的方式就是让你的团队记录下那些在代码审查过程中临时发现的问题,有了这些数据,你就能够确定你的团队常犯的错误,然后你就可以量身定制一个审查清单。确保你删除了那些没有出现过的错误。(你也可以保留那些出现概率很小,但是非常关键的项目,比如安全相关的问题)。

得到认可并且保持更新

基本规则是,清单上的任何条目都必须明确,而且,如果可能的话,对于一些条目你可以对其进行二元判定。这样可以防止判断的不一致。和你的团队分享这份清单并且让他们认同你清单的内容是个好主意。同样的,要定期检查你的清单,以确保各条目仍然是有意义的。

有了一个好的清单,可以提高你在代码审查过程中发现的缺陷个数。这可以帮助你提高代码标准,避免质量参差不齐的代码审查。


Peer code reviews are the single biggest thing you can do to improve your code.

General

  • Does the code work? Does it perform its intended function, the logic is correct etc.
  • Is all the code easily understood?
  • Does it conform to your agreed coding conventions? These will usually cover location of braces, variable and function names, line length, indentations, formatting, and comments.
  • Is there any redundant or duplicate code?
  • Is the code as modular as possible?
  • Can any global variables be replaced?
  • Is there any commented out code?
  • Do loops have a set length and correct termination conditions?
  • Can any of the code be replaced with library functions?
  • Can any logging or debugging code be removed?

Security

  • Are all data inputs checked (for the correct type, length, format, and range) and encoded?
  • Where third-party utilities are used, are returning errors being caught?
  • Are output values checked and encoded?
  • Are invalid parameter values handled?

Documentation

  • Do comments exist and describe the intent of the code?
  • Are all functions commented?
  • Is any unusual behavior or edge-case handling described?
  • Is the use and function of third-party libraries documented?
  • Are data structures and units of measurement explained?
  • Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like ‘TODO’?

Testing

  • Is the code testable? i.e. don’t add too many or hide dependencies, unable to initialize objects, test frameworks can use methods etc.
  • Do tests exist and are they comprehensive? i.e. has at least your agreed on code coverage.
  • Do unit tests actually test that the code is performing the intended functionality?
  • Are arrays checked for ‘out-of-bound’ errors?
  • Could any test code be replaced with the use of an existing API?

Conclusion

Optimize Your Checklist

Using the checklist as a starting point, you should optimize it for your specific use-case. A great way to do this is to get your team to note the issues that arise during code reviews for a short time. With this data, you’ll be able to identify your team’s common mistakes, which you can then build into a custom checklist. Be sure to remove any items that don’t come up (you may wish to keep rarely occurring, yet critical items such as security related issues).

Get Buy-in and Keep It Up To Date

As a general rule, any items on the checklist should be specific and, if possible, something you can make a binary decision about. This helps to avoid inconsistency in judgments. It is also a good idea to share the list with your team and get their agreement on its content. Make sure to review the checklist periodically too, to check that each item is still relevant.

Armed with a great checklist, you can raise the number of defects you detect during code reviews. This will help you to drive up coding standards and avoid inconsistent code review quality.


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. Sure, style and formatting form part of that, but you should let the tool be the one to point out those things.

Everyone should code review

Some people are better at it than others. The more experienced may well spot more bugs, and that’s important. But more important is maintaining a positive attitude to code review in general and that means avoiding any ‘Us vs. Them’ attitude, or making reviewing code burdensome for someone.

Review all code

No code is too short or too simple. If you review everything then nothing gets missed. What’s more, it makes it part of the process, a habit and not an after thought.

Adopt a positive attitude

This is just as important for reviewers as well as submitters. Code reviews are not the time to get all alpha and exert your coding prowess. Nor do you need to get defensive. Go in to it with a positive attitude of constructive criticism and you can build trust around the process.

For reviewers:

Code review often and for short sessions

The effectiveness of your reviews decreases after around an hour. So putting off reviews and doing them in one almighty session doesn’t help anybody. Set aside time throughout your day to coincide with breaks, so as not to disrupt your own flow and help form a habit. Your colleagues will thank you for it. Waiting can be frustrating and they can resolve issues quicker whilst the code is still fresh in their heads.

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

Don’t get picky, you don’t have to find an issue in every review.

Use a checklist

Code review checklists ensure consistency – they make sure everyone is covering what’s important and common mistakes.

For submitters:

Keep the code short

Beyond 200 lines and the effectiveness of a review drops significantly. By the time you’re at more than 400 they become almost pointless.

Provide context

Link to any related tickets, or the spec. There are code review tools like Kiln that can help with that. Provide short, but useful commit messages and plenty of comments throughout your code. It’ll help the reviewer and you’ll get fewer issues coming back.

ObjcZL
Hey There!
What is This?