はじめに
リファクタリングでシステムを安定化！日頃の取り組みや工夫を教えてください by カオナビ Advent Calendar 2022の9日目の記事を執筆しますMLLBと申します！
本記事では、コードの品質を保つ、または、改善していく上で、簡単かつ重要なTipsについて紹介していきたいと思います。
この記事を読んだ人が以下のようになることを想定しています。
- リファクタリングって何をやったらいいの？って人がリファクタリングの方法を知る
- リファクタリングの心理的ハードルが高いためできないという人でも、「これならリファクタリングできそう」という方法を知る
- リファクタリングの時間がない人が少しの時間でできるリファクタリングの方法を知る
上記を実現するために、簡単という部分にフォーカスしてみました。
本記事を書くモチベーション
前書きが長くなってしまうので以下は飛ばしていただいても大丈夫です！！
が、本記事を書くモチベーションとなった個人的な思いを綴らせていただきました。
個人的な経験の話になりますが、設計やリファクタリング、コードの書き方などを学んでも、これまで活かせない場面が多々ありました。
仕事で開発をする際、既存のプロジェクトのコードを触る機会が多かったです。また、新規開発であったとしても、アーキテクチャやフレームワーク、フォルダ構成などは、過去の開発を流用し、ある程度決められたものであることが多いような気がします。
つまり、自分の裁量でアーキテクチャやフォルダ構成、フレームワークなど、大きな変更・選定をする機会はあまりないと思ってます。
そういった事情から、仕事で開発をしていて、このアーキテクチャを使いたいのに使えないとか、フォルダ構成やクラス設計が気に入らなかったり、不満を抱いていました。でも、大きな変更をする工数がそもそもない(裁量権もない)、テストコードがないため気軽に試せないとか、多々障害がありました。
理解のある開発会社であれば、こういったことはないと思います。しかし、残念ながら理解のない会社、プロジェクトもあります（納期とか予算の関係でどうしてもできない場合もありますし）。そのような状況でも、簡単にできて効果が高いものを紹介させていてだこうと思いました。
また取り組むときの精神的負担が少なくしたいというのもモチベーションとなりました。
特に初心者の方や、プロジェクトに参画したばかりだと、こういう書き方がいいと思うけど、あえてこういう書き方してるのかなとか、そういうコーディング規約・文化なのかなとか、変更するのに障壁があるという人もいるとおもいます。(まあ、質問するのが一番いいんですが、雰囲気によっては聞きづらい場合もあるかと思います。)
なので、簡単な変更だと気兼ねなく取り組めるというメリットがあると思います。
目次
- ボーイスカウトルール
- コードを成形する
- コードを段落に分ける
- 変数名、メソッド名のタイポを修正する
- コメントを書く
- コメントを修正する
- 変数名、メソッド名を変更する
- 説明用変数を導入する
- 再代入不可にする
- 参照のない変数、メソッドを削除する
- 参考文献
ボーイスカウトルール
本記事のTipsはボーイスカウトルールの中で活用するのが使いやすいと思います。
ボーイスカウトルールとは、ボーイスカウトの活動のルールをプログラミングに当てはめた考え方です。
Robert C. Martin氏によって提唱されたようです。
プログラマが知るべき97のことの8で紹介されています。
本来のボーイスカウトルールは、「来た時よりも美しく」という内容で、山やキャンプ場に来た状態よりも、帰るときに綺麗な状態にするというルールです。
プログラミングに当てはめると、コードを触る前よりも、コードを触った後は綺麗な状態にするというルールになります。
例えば、機能変更や不具合修正で、既存のメソッドを修正しなければならなくなったとします。その時に、機能や不具合の修正をするだけじゃなくて、ファイル内にある使ってない変数を消したり、誤字を修正したりと、修正前よりもファイルが綺麗になるようにするということです。
では、綺麗にするって具体的に何をすればいいのかを紹介するのが本記事の趣旨なので、次章以降で具体例を挙げていきます。
コードを成形する
リファクタリング一つ目はコードを成形するです。
ロジックは変わらず、見た目の変更のみなので、心理的ハードルが低いかと思います。
リファクタリング前
@override
Widget build(BuildContext context) {
return Column(
mainAxisAlignment: MainAxisAlignment.start,
crossAxisAlignment: CrossAxisAlignment.start,
children: [Text("商品名：${orderData.productName}"),
Text("商品金額：${orderData.productPrice}"),
Text("注文日時：${dtFormat.format(orderData.dateTime)}"),
const Text("注文状況：配達中"),],
);
}
void output(String log)
{
print(log);
}
リファクタリング後
@override
Widget build(BuildContext context) {
return Column(
mainAxisAlignment: MainAxisAlignment.start,
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Text("商品名：${orderData.productName}"),
Text("商品金額：${orderData.productPrice}"),
Text("注文日時：${dtFormat.format(orderData.dateTime)}"),
const Text("注文状況：配達中"),
],
);
}
void output(String log) {
print(log);
}
コードが汚いとダメな理由
例に挙げたコードは記述量が少ないためほんのちょっとの差ですが、無頓着なプロジェクトだと未成型のコードが多く、汚く見えることがあります。汚いコードがあちこちにあると、割れ窓理論
という理論があるように、少々汚くてもいいやという心理が働きやすくなり、どんどん汚くなっていきやすいと個人的に考えています。
従って、コードの整形は日頃から気をつけておくべきことだと思います。
具体的にやること
- インデントを揃える
- 改行位置を揃える
- プロジェクトのコーディング規約に従って、if文やメソッドの後の
{}の位置も揃える
エディタによっては保存時に自動整形してくれる機能もあるので積極的に利用するといいと思います。
コードを段落に分ける
リファクタリング二つ目はコードを段落に分けるです。
改行をいれるだけで、ロジックは変わらず、見た目の変更のみ。なので、これも心理的ハードルが低いかと思います。
段落は処理のまとまりで分けると可読性が上がります。
リファクタリング前
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
var totalPrice = productPrice * productCount;
if(DateTime.now().weekday == DateTime.sunday){
totalPrice -= 100;
totalPrice = totalPrice > 0 ? totalPrice : 0;
}
print("${totalLabel}${totalPrice}${suffixLabel}");
}
リファクタリング後
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
var totalPrice = productPrice * productCount;
if(DateTime.now().weekday == DateTime.sunday){
totalPrice -= 100;
totalPrice = totalPrice > 0 ? totalPrice : 0;
}
print("${totalLabel}${totalPrice}${suffixLabel}");
}
リファクタリング前に比べて、処理のかたまりが視認しやすくなり、何をやっているか明確になったかとおもいます！
変数名、メソッド名のタイポを修正する
リファクタリング三つ目は変数名、メソッド名のタイポを修正する
そのままなのでやり方については特に解説しませんが、開発において命名は非常に重要です。
命名の重要性
個人的に重要だと感じるのは以下の理由からです。
- 間違った名前がそのまま放置されていると、このプロジェクトでは適当な命名でもいいのかな？と雑な命名のクラスやメソッド、変数が増えていく可能性が高まる
- きちんと命名されているコードは名前から内容が読み取りやすく、コードを追う時間が短くなる
- 勘違いが生まれる可能性がある（実体と命名が合ってないと、詳細まで精読しないとコードの意図がわからず、命名だけで処理を推測すると全然違う処理なことがある）
なので、タイポは見つけ次第直して行きましょう。
エディタによってはタイポがあれば強調してくれるものもあるので、これも積極的に利用して行きたいですね。
リファクタリング前
class TestScoreData {
int math;
int histry;
int sciense;
int calclate() {
return math + histry + sciense;
}
}
リファクタリング後
class TestScoreData {
int math;
int history;
int science;
int calculate() {
return math + histry + sciense;
}
}
コメントを書く
続いてのリファクタリングは、コメントを書くです。
コメントを追加しても、ロジックは変わりませんが、コードを理解しやすくしたり、コードだけでは意図が伝わりにくい箇所で意図を伝えることができます。
コメントの書き方については以下の書籍が大変勉強になるのでおすすめです。
読みやすいコードのガイドライン -持続可能なソフトウェア開発のために
個人的なポイントは以下です。
- ドキュメンテーションコメントはなるべく書く
- コードを段落に分けるという章で紹介した段落ごとにコメントを書く
- TODOなど、アノテーションコメントを書く
リファクタリング前
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
var totalPrice = productPrice * productCount;
if(DateTime.now().weekday == DateTime.sunday){
totalPrice -= 100;
totalPrice = totalPrice > 0 ? totalPrice : 0;
}
print("${totalLabel}${totalPrice}${suffixLabel}");
}
リファクタリング後
/// 合計料金を出力する
/// 日曜日の場合、１００円割引される
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
// 合計金額を計算
var totalPrice = productPrice * productCount;
// 日曜日の場合100円割引する
if(DateTime.now().weekday == DateTime.sunday){
totalPrice -= 100;
totalPrice = totalPrice > 0 ? totalPrice : 0;
}
// 合計金額を出力
print("${totalLabel}${totalPrice}${suffixLabel}");
}
コメントを修正する
章題の通りですが、コードの修正や変更が重なるとコメントと処理の内容が乖離してしまうケースがあります。自論ですが、コメントもコードの一部という意識を持った方が良いと考えています。
コメントの重要性について
コメントが誤っていてもコードが正しければシステムは動作します。そういう意味ではコメントは必要ないし、間違っていても問題ないとも言えます。
ただ今はそれで良くても、先のことを考えるとその状態は良くないと思います。
乖離していると良くない理由は以下です。
- コードリーディングの際、コメントを信用すると間違った解釈をしてしまう
- コードとコメントが違うと、不具合か？など違和感を覚え、いちいち疑いながら読んでいると効率が落ちる
こういった問題を未然に防ぐためにも、コメントはコードの一部と考え、コードを修正、変更した場合はコメントも修正、変更をするという癖をつけた方がいいと考えました。
日頃のコードを触る際にも、処理と乖離しているコメントは見つけ次第修正しましょう。
リファクタリング前
下記のサンプルコードでは、割引する曜日が月曜日になり、割引額が１５０円になったとします。リファクタリング前ではコードのみ修正しコメントはそのまま。リファクタリング後はコメントも修正しています。
/// 合計料金を出力する
/// 日曜日の場合、１００円割引される
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
// 合計金額を計算
var totalPrice = productPrice * productCount;
// 日曜日の場合100円割引する
if(DateTime.now().weekday == DateTime.monday){
totalPrice -= 150;
totalPrice = totalPrice > 0 ? totalPrice : 0;
}
// 合計金額を出力
print("${totalLabel}${totalPrice}${suffixLabel}");
}
リファクタリング後
/// 合計料金を出力する
/// 月曜日の場合、１5０円割引される
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
// 合計金額を計算
var totalPrice = productPrice * productCount;
// 日曜日の場合150円割引する
if(DateTime.now().weekday == DateTime.monday){
totalPrice -= 150;
totalPrice = totalPrice > 0 ? totalPrice : 0;
}
// 合計金額を出力
print("${totalLabel}${totalPrice}${suffixLabel}");
}
変数名、メソッド名を変更する
コメントの修正と似たような趣旨となりますが、実体と合ってない変数名とメソッド名も修正しましょう。
理由は以下の二つです。
- 命名は重要
- コードと命名が乖離しているとコメントの時と同じく読む際に困る
リファクタリング前
class Book {
final int id;
final String name;
Book({required this.id, requierd this.name,});
}
// 元々本があるか調べるだけのメソッドだったが、
// 本が借りれるかを返すように誰かが変更してしまったメソッドとする。
bool bookExists(int bookId){
// 元のコードはこんなかんじ。
// final List<Book> bookList = BookRepository().getBookList();
// return bookList.any((book) => book.id == bookId);
// 下のコードに誰かが変更したと仮定
final List<Book> bookList = BookRepository().getUnborrowedBookList();
return bookList.any((book) => book.id == bookId);
}
リファクタリング後
class Book {
final int id;
final String name;
Book({required this.id, requierd this.name,});
}
bool canBorrow(int bookId){
final List<Book> unborrowedBookList = BookRepository().getUnborrowedBookList();
return unborrowedBookList.any((book) => book.id == bookId);
}
説明用変数を導入する
説明用変数とは要は変数なのですが、あるメソッドの戻り値をそのまま引数にいれたりせずに意図の分かるように変数に入れるというテクニックになります。
ちょっと言葉じゃ分かりにくいので下記に例を示します。
リファクタリング前
int calculateTotalPrice(int productPrice, int productCount){
var totalPrice = productPrice * productCount;
if(DateTime.now().weekday != DateTime.sunday){
return totalPrice;
}
totalPrice -= 100;
return totalPrice > 0 ? totalPrice : 0;
}
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
print("${totalLabel}${calculateTotalPrice(productPrice, productCount)}${suffixLabel}");
}
リファクタリング後
int calculateTotalPrice(int productPrice, int productCount){
var totalPrice = productPrice * productCount;
if(DateTime.now().weekday != DateTime.sunday){
return totalPrice;
}
// 割引金額を変数化して説明
int discountPrice = 100;
// メソッドの先頭で計算したtotalPriceをそのまま使わず、違う変数に切り出して説明
int discountedPrice = totalPrice - discountPrice;
return discountedPrice > 0 ? discountedPrice : 0;
}
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
// ここでいったん変数に格納し、合計金額であることをわかりやすくする。
int totalPrice = calculateTotalPrice(productPrice, productCount);
print("${totalLabel}${totalPrice}${suffixLabel}");
}
どこを変えたかはリファクタリング後のコメントで説明を入れました。
例が簡単なので劇的な効果があるか疑問に思う方もいるかもしれませんが、利点は以下です。
- （特に複雑な処理の場合)変数で内容を明示しているので読みやすい
- コードを書く側も、今書いているコードの意図や目的を変数として命名するという作業の中で何をしようとしているのか明確になる(というか明確にしないと適切な命名ができません)
再代入不可にする
基本的に変数は再代入不可にしておくのが良いです。
いままで学んできた限りでは、データモデルにしても、クラスのメンバ変数にしてもイミュータブルにしておくのが良いという考えが多かったです。
なので、メソッド内で使う変数もできる限り再代入不可に変えましょう。
再代入不可にするメリット
- 読む側は変数の値の変更箇所に気を使いながら読む必要がなくなりますし、
- コードの修正・変更のタイミングで間違った使い方をされたり、値を書き換えられて動作がおかしくなったりみたいなことを防げます。
リファクタリング前
(本記事はdartで書いておりますので、変数の再代入不可はfinalか定数のconstになります)
int calculateTotalPrice(int productPrice, int productCount){
var totalPrice = productPrice * productCount;
if(DateTime.now().weekday != DateTime.sunday){
return totalPrice;
}
int discountPrice = 100;
int discountedPrice = totalPrice - discountPrice;
return discountedPrice > 0 ? discountedPrice : 0;
}
void outputTotalPrice(int productPrice, int productCount){
String totalLabel = "合計：";
String suffixLabel = "円です";
int totalPrice = calculateTotalPrice(productPrice, productCount);
print("${totalLabel}${totalPrice}${suffixLabel}");
}
リファクタリング後
int calculateTotalPrice(int productPrice, int productCount){
final int totalPrice = productPrice * productCount;
if(DateTime.now().weekday != DateTime.sunday){
return totalPrice;
}
const int discountPrice = 100;
final int discountedPrice = totalPrice - discountPrice;
return discountedPrice > 0 ? discountedPrice : 0;
}
void outputTotalPrice(int productPrice, int productCount){
const String totalLabel = "合計：";
const String suffixLabel = "円です";
final int totalPrice = calculateTotalPrice(productPrice, productCount);
// 再代入不可にしておくことで、例えば上の行と下の行の間に長い処理が入ったとしても、
// totalPriceは書き換えられないことが保証されるので、
// 読む側はこの間にtotalPriceの値に関して気にする必要がない
// またこのメソッドに修正・変更が加えられたとしても、
// totalPriceに間違って値を代入されたり変更したりされることも防げる
print("${totalLabel}${totalPrice}${suffixLabel}");
}
参照のない変数、メソッドを削除する
変更、修正、機能追加などコードが変更されるにつれて、参照されなくなった変数やメソッドが多々出てきます。
そうした変数やメソッドはどんどん削除していっていいと思います。
ただメソッドに関しては本当に削除してしまっていいのか、将来使うことがあるかもしれないといった考えもあると思いますが、
YAGNIの原則に照らし合わせて、現在必要な機能でなければ別に削除してしまってもいいのかなーと考えます。結局こういうメソッドは保守されずに放置されるので、残しておいてもそのままでずっと使えるのかも分かりませんし。
リファクタリング前
// 元々りんご、ばなな、オレンジを扱っていたが、オレンジは扱わないことになり、合計金額の計算からは除外されたものの、
// オレンジの料金と計算するメソッドはさくじょされなかったとします
class FruitsCalculator {
static const int applePrice = 100;
static const int bananaPrice = 200;
static const int orangePrice = 300;
int totalPrice (int appleCount, int bananaCount) {
final int appleTotalPrice = calculateAppleTotalPrice(appleCount);
final int bananaTotalPrice = calculateBananaTotalPrice(appleCount);
return appleTotalPrice + bananaTotalPrice;
}
int calculateAppleTotalPrice (int appleCount) {
return appleCount * applePrice;
}
int calculateBananaTotalPrice (int bananaCount) {
return bananaCount * bananaPrice;
}
int calculateOrangeTotalPrice (int orangeCount) {
return orangeCount * orangePrice;
}
}
リファクタリング後
class FruitsCalculator {
static const int applePrice = 100;
static const int bananaPrice = 200;
int totalPrice (int appleCount, int bananaCount) {
final int appleTotalPrice = calculateAppleTotalPrice(appleCount);
final int bananaTotalPrice = calculateBananaTotalPrice(appleCount);
return appleTotalPrice + bananaTotalPrice;
}
int calculateAppleTotalPrice (int appleCount) {
return appleCount * applePrice;
}
int calculateBananaTotalPrice (int bananaCount) {
return bananaCount * bananaPrice;
}
}
必要ない部分が削除されてスッキリしますし、今の仕様を的確に表しているのはリファクタリング後のコードです。
何ができるかと同じくらい、何ができないかも重要で、リファクタリング前のコードでは一見オレンジに関するコードが残っているので、オレンジについてもこのシステムでは取り扱えるという勘違いを生む原因にもなります。そのクラスでできないことはコードとしても表現しないようにすることで、逆にコードの表現力は上がります。(何かの本か記事にこういう趣旨のことが書かれていたのですが忘れました。もしご存知の方がいればコメントで教えてください)