前提
今の会社に入社して3ヶ月になりました。
一人で開発するのとは異なり、
チームの人にコードを見てもらうということを意識して、
ソースコード管理を行う必要がでてきました。
そのため、コミットの粒度は小さくすることが
チームに見てもらう上で大事だと実感しました。
なぜ小さい粒度のコミットがいいのか？
大きいコミットより、小さいコミットの方が見やすく、
変更箇所もわかりやすいからです。
またチームの方にとっては、
より小さい変更の方がコードをレビューする際に心理的負担も減ります。
ただ、そうは言われていても、
小さいってどれくらいだよ、という問題にぶちあたります。
(例外にもれず、僕もです)
この問題は、コミットメッセージから考える ことでスッキリしました。
コミットメッセージから考える
コミットメッセージには一つの追加/修正についてのみ書いていきます。
- スネークケースをキャメルケースに変換する処理を追加
- 正規表現のグローバルオプションを修正
上記のように実装する前に、
今回のコミットで何を追加/修正したいのかメッセージを考えていきます。
終わりから始めることで、
何をこのコミットでやるべきことなのかを明確にしていきます。
そうすることで、今のコミットに関係のない機能を追加したり、
リファクタリングをしたくなる誘惑からも解放されると思います。
また、コミットメッセージには一つのことを書くことが大事になります。
例えば、AとBの機能を追加のように書くのはあまりよくありません。
コミットメッセージの書き方はこの記事では扱わないので、以下を参考にしてみてください。
参考記事
コミットの実践
実際にコミットを作って説明していきたいと思います。
マスターブランチ
masterブランチに以下のようなコードがあります。
const snakeToCamel = (str) => {
const reg = /_([a-z])/;
return str.replace(reg, (matched) => matched.slice(1).toUpperCase())
}
const camelToSnake = (str) => {
console.log(str);
const reg = /([A-Z])/;
return str.replace(reg, (matched) => "_" + matched.toLowerCase())
}
export {
snakeToCamel,
camelToSnake
}
また、Stringを渡したら自動でキャメルケースとスネークケースを変換する
という機能の追加があるとします。
上記のコードと、仕様変更を考えると以下がタスクになりそうです。
- 正規表現のバグ修正
- 新しい機能の追加
- リファクタリング
BAD
よくやってしまう良くないコミットは、
複数のこと(バグ修正、機能追加、リファクタリングなど)を
ひとつのコミットに含めてしまうことです。
+ const snakeReg = /_([a-z])/g;
+ const camelReg = /([A-Z])/g;
const snakeToCamel = (str) => {
- const reg = /_([a-z])/;
- return str.replace(reg, (matched) => matched.slice(1).toUpperCase())
+ return str.replace(snakeReg, (matched) => matched.slice(1).toUpperCase())
}
const camelToSnake = (str) => {
- console.log(str);
- const reg = /([A-Z])/;
- return str.replace(reg, (matched) => "_" + matched.toLowerCase())
+ return str.replace(camelReg, (matched) => "_" + matched.toLowerCase())
}
+ const convertSpelling = (str) => {
+ if(snakeReg.test(str)){
+ return snakeToCamel(str);
+ }
+ if(camelReg.test(str)){
+ return camelToSnake(str);
+ }
+ return str
+ }
export {
snakeToCamel,
- camelToSnake
+ camelToSnake,
+ convertSpelling
}
なぜこのように複数のことをやるコミットが良くないのかと言うと、
- このコミットで何を行ったのかチームの人にわかりづらくなる。
- 副作用やバグが出たとき、原因がわかりづらくなる。
- バグ修正と機能追加などを一つのコミットで行うと、ソースコードを戻したい時などに、デグレード(バグが復活)してしまうことがある。
などが挙げられます。
なのでなるべく、小さいコミットで行うほうがいいと思います。
GOOD
ここからは、先ほどのコミットを
コミットメッセージから複数に分けていきたいと思います。
FIX
バグ修正はバグ修正のみのコミットを行います。
余計なことはせずにすぐに修正して適応します。
コミットを見たときに、バグを修正したとチームの方にも分かりやすいです。
また、他のバグが出てくる可能性をなるべくゼロにして開発したほうがいいと思います。
コミットメッセージはこんな感じ
正規表現のグローバルオプションを修正
- const reg = /_([a-z])/;
+ const reg = /_([a-z])/g;
- const reg = /([A-Z])/;
+ const reg = /([A-Z])/g;
新規機能追加と一緒に改善してしまうこともできますが、
もし新規機能がいらなくなり、元に戻すことになると、
バグが再発(デグレード)してしまうこともあります。
なので、一つのコミットに一つのやったことが大事になります。
新規機能追加
次に、渡されたStringがキャメルケースか、スネークケースかを検出し、
変換する処理を追加するコミットです。
コミットメッセージはこんな感じ
検出されたスペルによって変換する処理を追加
...
...
+ const convertSpelling = (str) => {
+ if(/_([a-z])/g.test(str)){
+ return snakeToCamel(str);
+ }
+ if(/([A-Z])/g.test(str)){
+ return camelToSnake(str);
+ }
+ return str
+ }
export {
- snakeToCamel,
- camelToSnake
+ convertSpelling
}
リファクタリング
最後にconsole.logの削除や、
何度も使いまわしている正規表現を外に出すようにしていきます。
コミットメッセージはこんな感じ
snakeReg関連をリファクタリング
+ const snakeReg = /(_[a-z])/g;
+
const snakeToCamel = (str) => {
- const reg = /_([a-z])/g;
- return str.replace(reg, (matched) => matched.slice(1).toUpperCase())
+ return str.replace(snakeReg, (matched) => matched.slice(1).toUpperCase())
}
...
...
const convertSpelling = (str) => {
- if(/_([a-z])/g.test(str)){
+ if(snakeReg.test(str)){
return snakeToCamel(str);
}
...
...
次がこんな感じ
camelReg関連をリファクタリング
const snakeReg = /(_[a-z])/g;
+ const camelReg = /([A-Z])/g;
...
...
const camelToSnake = (str) => {
- console.log(str);
- const reg = /([A-Z])/g;
- return str.replace(reg, (matched) => "_" + matched.toLowerCase())
+ return str.replace(camelReg, (matched) => "_" + matched.toLowerCase())
}
const convertSpelling = (str) => {
...
...
- if(/([A-Z])/g.test(str)){
+ if(camelReg.test(str)){
return camelToSnake(str);
}
...
...
コミットの全体
camelReg関連をリファクタリング
snakeReg関連をリファクタリング
検出されたスペルによって変換する処理を追加
正規表現のグローバルオプションを修正
リファクタリングを先に行い正規表現を外に出してから、
新規機能を追加するでもいいと思います。
ここらへんは、人やチームそれぞれな気がします。
プルリクの実践
先ほどまでのコミットを用いて、プルリク作成していきたいと思います。
運用方法についてはこの記事では扱いません。
プルリクではレビュアーにとって見やすい最小のやったこと
でプルリクを作るといいと思います。
この最小のやったことが何かという問題も、プルリクのタイトルメッセージから考えればスッキリすると思います。
先ほどのコミットでは3つのことをやっています。
- バグの修正
- 機能の追加
- リファクタリング
機能の追加 と リファクタリング は別のやったことですが
レビュアーにとって見やすい最小のやったこと、
で考えると、まとめてしまっても良いのかなと思います。
コミットはもちろん分けます。
console.logの削除やtypo修正、変数をまとめたりなど
変更が小さいものなら、一つにまとめてしまっていいと思います。
しかし、ディレクトリ構造の変更や、関数をまとめてしまうなどといった、
大きなリファクタリングは最小のやったこととして成立するので、
別途プルリクを作成した方が良いと思います。
BAD
以下のようなプルリクはあまり良くありません。
- 機能の追加とバグの修正
というのも、
- バグの修正ができていても、機能の追加でバグが見つかったときにマージできず、修正しなければならない
- 機能追加がいらなくなったとき、バグ修正も入っているのでクローズにできない
などの問題が生じてしまいます。
ここらへんはコミット部分にも通じるものがあります
GOOD
今回の例では以下のような感じでやるといいと思います。
- 正規表現の修正
- 自動でキャメルケースとスネークケースを変換する処理を追加
バグ修正のプルリクと、機能追加のプルリクを作ります。
こうすることで、すぐに反映する必要があるバグ修正のプルリクを
レビューして、問題なければすぐにマージすることができます。
また機能追加のプルリクも独立しているため、
仕様変更などがあっても他の開発に支障があまりありません。
参考記事
「巨大プルリク1件vs細かいプルリク100件」問題を考える（翻訳）
Working while waiting for pending PR (プルリクを作ったブランチを用いて作業したいとき)
まとめ
終わり(コミットメッセージやプルリクのメッセージ)から始めることで、
誘惑からも解放され、
なるべく小さいコミットにすることができるようになると思います。
最後までありがとうございました。
レビューしていただけると嬉しいです