75
74

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 5 years have passed since last update.

良いエンジニアがつくる Diff は綺麗だ

Last updated at Posted at 2016-07-25

最近メンティーの成果物をレビューしていて,彼らにちょっと意識してほしいなと思ったので,メモ.

Diff とは

Linux の複数のファイルの差分を可視化する diff コマンドから派生して,エンジニア界隈では2つの成果物の差分を指す事が多い.
なお,本稿では Diff を話題にあげているが,Git などのバージョン管理システム (VCS) 上でのコミットログと言っても大差ないかと思う.

なぜ Diff が綺麗だと嬉しいか

敢えて上げる必要も無いかもしれないが,ぱっと思いついたものを挙げてゆく.

  • ただ単純にレビューがしやすい
    99% はここに行き着く.GitHub 上でプルリクエストを受け取った時に「この修正は何を意図しているか」と「実際にどのような修正を行ったか」の間を対応させるのに無駄な労力を使わずに済めば,その修正意図の是非や修正方法の良し悪しといった,より本質的なことに時間を割くことが出来る.
  • Git 上でコミットの整理をし易い
    Git の操作方法を解説するつもりはないので別稿にゆずるが,git add -p で変更の一部分をコミットしたり,git rebase -i でコミットの順番を替えるなど,一度行った作業ログを整理することが出来る.良い Diff をつくってこまめにコミットしていれば,このような操作が楽になる.

どうすれば綺麗な Diff を作れるようになるか

一朝一夕ではないが,ヒントになれば.

行を分ける.

ファイル間の Diff は行単位で見ることが多い.1つの意味と1つの行が一対一に対応するようにコーディングすると,後で何かと嬉しい.

例1

Gradle でマルチプロジェクトによるビルド構成を組み立てる際,settings.gradle というファイルにてプロジェクトに内包されるサブプロジェクトを定義する.以下のように書くこともできるが,あまり良いとは思えない.

settings.gradle
include 'database', 'domain', 'api'

なぜなら,このプロジェクトに新しくサブプロジェクトを追加するとなった際に,何が増えたかわかりづらいからだ.

settings.gradle
+include 'database', 'customer-domain', 'order-domain', 'api'
-include 'database', 'domain', 'api'

文字数が少々増えると分かっていても,以下のように書くのが望ましい.

settings.gradle
include 'database'
include 'domain'
include 'api'

これなら,どの行が増えて,どの行が減ったかが一目瞭然である.

settings.gradle
 include 'database'
-include 'domain'
+include 'customer-domain'
+include 'order-domain'
 include 'api'

例2

これはやり過ぎの感もあって好みも分かれるが,試みとしてはアリ.
Ruby on Rails のコントローラー上にてリクエストパラメータにフィルタを掛ける (Strong Parameter) が,これも引数の増減を行単位で可視化させることが出来る.

普通の例.rb
params.require[:model].permit(:id, :name, :address, :score)

もうおわかりかと思うが,引数が増えた時に何が何だかパッと見でわからない.

普通の例.rb
-params.require[:model].permit(:id, :telephone, :address, :score)
+params.require[:model].permit(:id, :name, :address, :postal, :score)

以下のように行を分けてしまえば,何か足し忘れたり引き忘れたりするのも防げそう.

改善例.rb
 params.require[:model].permit(:id,
-                              :telephone,
+                              :name,
                               :address,
+                              :postal,
                               :score)

ファイルを分ける

コミット間 Diff (あるいは,単にコミットログ) を確認する際には,行単位の比較もそうだが,「どのファイルに差分が存在するか」というファイル単位の差分もよく見る.
ファイル単位の差分の理解が容易になると,後から確認したりレビューするときに余計な負担がなくなる.

Ruby on Rails にて表示文言の国際化を図るライブラリといえば i18n が定番だ.
Rails のデフォルトでは,(ロケール名).yml というファイルでメッセージリソースを定義するだろう.

ja.yml
 ja:
   customer:
     name: 名前
     address: 住所
     category: 顧客区分
 #
 # この間に 1,000 行くらい………
 #
   order:
-    id: 注文ID
+    id: 注文明細ID
     date: 発注日
 #
 # さらにこの間に 1,200 行くらい……
 #
   user:
     name: ユーザー名
-    created_at: 作成日
+    created_at: 登録日時
     upadted_at: 更新日時
#
# この後にも 600 行くらい

これだけ大きいファイルにすべての日本語ロケールのメッセージリソースが含まれているとなると,複数人でアプリケーション開発をした際には変更の競合が発生しやすくなるし,変更の過不足にも気づきにくくなる.
ファイルを分けて,複数のファイルを i18n に読み込んでもらうように修正することで,保守性が高くなる.

customer.ja.yml
 ja:
   customer:
     name: 名前
     address: 住所
     category: 顧客区分
order.ja.yml
 ja:
   order:
-    id: 注文ID
+    id: 注文明細ID
     date: 発注日
user.ja.yml
 ja:
   user:
     name: ユーザー名
-    created_at: 作成日
+    created_at: 登録日時
     upadted_at: 更新日時

VCS や Diff ツールにかければ,customer.ja.yml には変更がなくて order.ja.yml と user.ja.yml にのみ変更があると出るはずなので,見返すのがラク.

コーディングスタイルを揃える

Diff の話に限らないが,やはり気をつけたい.

例1) フォーマット

bad.json
 [
   {
     "name": "石井琢朗",
     "number": 5,
     "position": "Short Stop"
   },
-  {
-    "name": ”波留敏夫",   
-    "number": 1,
-    "position": "Center Fielder"
+    {"name": "波留敏夫",
+      "number": 2,
+      "position": "Center Fielder"
   }
 }

上記の Diff の内容は,波留の背番号が 2 から 1 に変わったこと (と,フォーマットを揃えたこと) だけだ.やはり "number" の行だけに変更が出るようにしたい.
Eclipse 上での Java 開発なら Save Action (保管アクション) でフォーマットをかけるように,JetBrains 系 IDE ならコミット前にフォーマッティングをかけるようにしたい.

例2) コード規約

Checkstyle や Linter などの静的解析ツールで防げるものもあるが,そうでないものも多いので日頃から気をつけたい.

例えば Java のフィールド変数を参照する際に this をつけるかなどは,好みが分かれる.

Example.java
 public class Example {
     private int a;
     private int b;

     /** 全く意味のない例で恐縮だが… */     
     void hoge(int c) {
-        int d = c - this.b;
+        int d = c + b;
         int e = c * this.a / Math.random();
         return Math.abs(d - e);
     }    
 }

本質的には d の計算に使う引き算を足し算に変えただけだが,意味もなく this.bb に変わっている.「ローカル変数に b ってあったっけ…??」などと思わせることがないようにしたい.
やはり,コミットをする前には Diff を確認して無駄な変更がないかを気にしたい.

参考文献


diff をハイライトできる Qiita Markdown に感謝.

75
74
1

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
75
74

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?