Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Heartless code review

Heartless code review

rejasupotaro

August 15, 2015
Tweet

More Decks by rejasupotaro

Other Decks in Technology

Transcript

  1. Heartless code review
    @rejasupotaro

    View full-size slide

  2. Why should we do
    code reviews?

    View full-size slide

  3. We are doing code reviews to
    * improve code quality
    * improve UX
    * share knowledge

    View full-size slide

  4. Problem of human code review
    * Depending on the skill
    * Depending on the mood of the day

    View full-size slide

  5. It seems that code quality
    can be checked automatically
    * Improve code quality
    * Improve UX
    * Share knowledge

    View full-size slide

  6. Code bad smell detection
    * Correctness
    * Bad practice
    * Malicious code vulnerability
    * Performance

    View full-size slide

  7. Use
    static analysis tools

    View full-size slide

  8. static analysis tools
    for Java
    * PMD
    * Checkstyle
    * FindBugs
    * Infer

    View full-size slide

  9. PMD & Checkstyle

    View full-size slide

  10. Use formatter

    View full-size slide

  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

    View full-size slide

  12. FindBugs
    Find 420+ types of bug
    Super powerful!
    Some reports are too strict
    (= false-positive)
    for Android

    View full-size slide

  13. developed by Facebook
    written in OCaml
    Infer
    A tool to detect bugs in Android and iOS app
    before they ship

    View full-size slide

  14. I chose FindBugs
    Let’s look more closely

    View full-size slide

  15. FindBugs is bundled
    with Gradle

    View full-size slide

  16. Just add `apply plugin: findbugs`

    View full-size slide

  17. 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

    View full-size slide

  18. $ ./gradlew findbugs

    View full-size slide

  19. Many warnings

    View full-size slide

  20. Look report closely

    View full-size slide

  21. 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:

    View full-size slide

  22. 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…

    View full-size slide

  23. Worthless indication
    makes engineers unmotivated

    View full-size slide

  24. Exclude
    Include
    write exclude-filter to keep engineer’s motivation

    View full-size slide

  25. 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

    View full-size slide










  26. exclude.xml

    View full-size slide











  27. exclude.xml

    View full-size slide

  28. 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

    View full-size slide

  29. Bug type: UPM_UNCALLED_PRIVATE_METHOD

    View full-size slide

  30. Bug type: URF_UNREAD_FEILD

    View full-size slide











  31. exclude.xml

    View full-size slide

  32. →Filter→

    View full-size slide

  33. Nothing is changed
    if you just look a result
    Call to action

    View full-size slide

  34. Use review tool

    View full-size slide

  35. Dokumi
    notify
    Pull Request
    CI Server
    run dokumi
    comment
    xcodebuild
    infer
    FindBugs
    Oh, I have to fix it!

    View full-size slide

  36. You can setup this system in 30 minutes!

    View full-size slide

  37. Advantage
    * Save the time of code review
    * Keep code high quality
    * Focus on essential problem

    View full-size slide

  38. Bots say harsh things
    to keep code quality high
    Human praise reviewee
    to motivate team members

    View full-size slide

  39. Thank you!
    @rejasupotaro

    View full-size slide