Heartless code review

Heartless code review

666ef10ec14e5a23d0fcf05bd2665575?s=128

rejasupotaro

August 15, 2015
Tweet

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

  38. None
  39. None
  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