Heartless code review

Transcript

1. Heartless code review @rejasupotaro
2. Why should we do code reviews?
3. We are doing code reviews to * improve code quality * improve UX * share knowledge
4. Problem of human code review * Depending on the skill * Depending on the mood of the day
5. It seems that code quality can be checked automatically * Improve code quality * Improve UX * Share knowledge
6. Code bad smell detection * Correctness * Bad practice * Malicious code vulnerability * Performance
7. Use static analysis tools
8. static analysis tools for Java * PMD * Checkstyle * FindBugs * Infer
9. PMD & Checkstyle
10. Use formatter
11. You can setup formatter with 1 command * Square: ./install.sh * Cookpad: curl -L "https://raw.githubusercontent.com/cookpad/android-code-style/ master/.idea/codeStyleSettings.xml" > .idea/codeStyleSettings.xml
12. FindBugs Find 420+ types of bug Super powerful! Some reports are too strict (= false-positive) for Android
13. developed by Facebook written in OCaml Infer A tool to detect bugs in Android and iOS app before they ship
14. looks WIP
15. I chose FindBugs Let’s look more closely
16. FindBugs is bundled with Gradle
17. Just add `apply plugin: findbugs`
18. findbugs { toolVersion = "2.0.1" sourceSets = [sourceSets.main] ignoreFailures = true reportsDir = file("$project.buildDir/findbugsReports") effort = "max" reportLevel = "high" visitors = ["FindSqlInjection", "SwitchFallthrough"] omitVisitors = ["FindNonShortCircuit"] includeFilter = file(“$rootProject.projectDir/…/includeFilter.xml”) excludeFilter = file(“$rootProject.projectDir/…/excludeFilter.xml”) } All properties have sensible defaults
19. $ ./gradlew findbugs
20. Many warnings
21. Look report closely
22. BuildConfig.class … R$anim.class R$attr.class R$bool.class R$color.class R$dimen.class R$drawable.class R$id.class … Code analyzed:
23. TopActivity$$ViewBinder.class SearchResultActivity$$ViewBinder.class RecipeDetailActivity$$ViewBinder.class PhotoReportActivity$$ViewBinder.class ProfileActivity$$ViewBinder.class LoginActivity$$ViewBinder.class SettingsActivity$$ViewBinder.class … We don’t need to analyse these classes…
24. Worthless indication makes engineers unmotivated
25. Exclude Include write exclude-filter to keep engineer’s motivation
26. <FindBugsFilter> </FindBugsFilter> exclude.xml
27. <FindBugsFilter> <Match> <Class name="~.*BuildConfig.*" /> </Match> <Match> <Class name=“~.*\$.*”/> </Match> </FindBugsFilter> exclude.xml
28. ClassName$InnerClassName.class … ClassName$$Lambda$1.class ClassName$$Lambda$2.class ClassName$$Lambda$3.class ClassName$$Lambda$4.class ClassName$$Lambda$5.class ClassName$$Lambda$6.class ClassName$$Lambda$7.class …
29. <FindBugsFilter> <Match> <Class name="~.*BuildConfig.*" /> </Match> <Match> <Class name=“~.*\$$ViewBinder.*”/> </Match> … </FindBugsFilter> exclude.xml
30. <FindBugsFilter> <Match> <Class name="~.*BuildConfig.*" /> </Match> <Match> <Class name=“~.*\$$ViewBinder.*”/> </Match> … <Bug code=“…”> </FindBugsFilter> exclude.xml
31. AlertDialog dialog = new AlertDialog.Builder(context) .setMessage(R.string.message) .setPositiveButton(R.string.ok, new DialogInterface.OnClickListener() { @Override public void onClick(DialogInterface dialog, int which) { … } }) .setNegativeButton(R.string.cancel, new DialogInterface.OnClickListener() { @Override public void onClick(DialogInterface dialog, int which) { … } }) .show(); Bug type: SIC_INNER_SHOULD_BE_STATIC_ANON
32. Bug type: UPM_UNCALLED_PRIVATE_METHOD
33. Bug type: URF_UNREAD_FEILD
34. <FindBugsFilter> <Match> <Class name="~.*BuildConfig.*" /> </Match> <Match> <Class name=“~.*\$$ViewBinder.*”/> </Match> … <Bug code=“SIC,UPM,UrF,UwF”> </FindBugsFilter> exclude.xml
35. →Filter→
36. Nothing is changed if you just look a result Call to action
37. Use review tool
40. Dokumi notify Pull Request CI Server run dokumi comment xcodebuild infer FindBugs Oh, I have to fix it!
41. You can setup this system in 30 minutes!
42. Advantage * Save the time of code review * Keep code high quality * Focus on essential problem
43. Bots say harsh things to keep code quality high Human praise reviewee to motivate team members
44. Thank you! @rejasupotaro