Heartless code review

Transcript

1. Heartless code review
   @rejasupotaro
   

   View Slide

2. Why should we do
   code reviews?
   

   View Slide

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

   View Slide

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

   View Slide

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

   View Slide

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

   View Slide

7. Use
   static analysis tools
   

   View Slide

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

   View Slide

9. PMD & Checkstyle
   

   View Slide

10. Use formatter
    

    View 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 Slide

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

    View Slide

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

    View Slide

14. looks
    WIP
    

    View Slide

15. I chose FindBugs
    Let’s look more closely
    

    View Slide

16. FindBugs is bundled
    with Gradle
    

    View Slide

17. Just add `apply plugin: findbugs`
    

    View Slide

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
    

    View Slide

19. $ ./gradlew findbugs
    

    View Slide

20. Many warnings
    

    View Slide

21. Look report closely
    

    View Slide

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:
    

    View Slide

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…
    

    View Slide

24. Worthless indication
    makes engineers unmotivated
    

    View Slide

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

    View Slide

26. 
    
    exclude.xml
    

    View Slide

27. 
    
    
    
    
    
    
    
    exclude.xml
    

    View Slide

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
    …
    

    View Slide

29. 
    
    
    
    
    
    
    …
    
    exclude.xml
    

    View Slide

30. 
    
    
    
    
    
    
    …
    
    
    exclude.xml
    

    View Slide

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
    

    View Slide

32. Bug type: UPM_UNCALLED_PRIVATE_METHOD
    

    View Slide

33. Bug type: URF_UNREAD_FEILD
    

    View Slide

34. 
    
    
    
    
    
    
    …
    
    
    exclude.xml
    

    View Slide

35. →Filter→
    

    View Slide

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

    View Slide

37. Use review tool
    

    View Slide

38. View Slide

39. View Slide

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

    View Slide

41. You can setup this system in 30 minutes!
    

    View Slide

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

    View Slide

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

    View Slide

44. Thank you!
    @rejasupotaro
    

    View Slide