Heartless code review - Speaker Deck

Slide 1

Slide 1 text

Heartless code review @rejasupotaro

Slide 2

Slide 2 text

Why should we do code reviews?

Slide 3

Slide 3 text

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

Slide 4

Slide 4 text

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

Slide 5

Slide 5 text

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

Slide 6

Slide 6 text

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

Slide 7

Slide 7 text

Use static analysis tools

Slide 8

Slide 8 text

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

Slide 9

Slide 9 text

PMD & Checkstyle

Slide 10

Slide 10 text

Use formatter

Slide 11

Slide 11 text

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

Slide 12

Slide 12 text

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

Slide 13

Slide 13 text

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

Slide 14

Slide 14 text

looks WIP

Slide 15

Slide 15 text

I chose FindBugs Let’s look more closely

Slide 16

Slide 16 text

FindBugs is bundled with Gradle

Slide 17

Slide 17 text

Just add `apply plugin: findbugs`

Slide 18

Slide 18 text

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

Slide 19

Slide 19 text

$ ./gradlew findbugs

Slide 20

Slide 20 text

Many warnings

Slide 21

Slide 21 text

Look report closely

Slide 22

Slide 22 text

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:

Slide 23

Slide 23 text

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…

Slide 24

Slide 24 text

Worthless indication makes engineers unmotivated

Slide 25

Slide 25 text

Exclude Include write exclude-ﬁlter to keep engineer’s motivation

Slide 26

Slide 26 text

exclude.xml

Slide 27

Slide 27 text

exclude.xml

Slide 28

Slide 28 text

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 …

Slide 29

Slide 29 text

… exclude.xml

Slide 30

Slide 30 text

… exclude.xml

Slide 31

Slide 31 text

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

Slide 32

Slide 32 text

Bug type: UPM_UNCALLED_PRIVATE_METHOD

Slide 33

Slide 33 text

Bug type: URF_UNREAD_FEILD

Slide 34

Slide 34 text

… exclude.xml

Slide 35

Slide 35 text

→Filter→

Slide 36

Slide 36 text

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

Slide 37

Slide 37 text

Use review tool

Slide 38

Slide 38 text

No content

Slide 39

Slide 39 text

No content

Slide 40

Slide 40 text

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

Slide 41

Slide 41 text

You can setup this system in 30 minutes!

Slide 42

Slide 42 text

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

Slide 43

Slide 43 text

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

Slide 44

Slide 44 text

Thank you! @rejasupotaro