コードレビューをしていて気になったこと（C#）
こんにちは、KENです。最近はコードを書くよりもコードレビューする機会が増えてきました。
コードレビューする中で、挙動は間違ってないけど、こう書いたらもっと見やすいコードにできるのではというものがありました。
今回は今までコードレビューした中で数行程度のものピックアップして、こうすれば良いのではという私の考えを紹介していきたいと思います。
言語はC#です。記載しているコードは例として書いたものになります。
目次
命名に関するもの
boolの変数名、メソッド名
下記のように〇〇Flgという変数名や、Check〇〇というメソッドをよく見ます。
updateFlgの名前から、trueが更新対象を意味しているのか、falseが更新対象を意味しているのかが判断できません。
また、CheckUpdateメソッドも、戻り値がtrueなのか、falseなのかがメソッドの中身を見ないと判断できないことが多いです。
(後続のif文から更新対象を示すのかなと思って、Checkメソッドの中身を見ると逆だったということがありました。)
var updateFlg = CheckUpdate();
if(updateFlg)
{
// 更新処理
}変数名、メソッド名からtrueかfalseかわかるような命名が良いと思います。
var isUpdatable = isUpdatable();
if(isUpdatable)
{
// 更新処理
}Listの変数名、メソッド名
下記のような単数形のリストをよく見ます。変数名、メソッド名からリストであることが分かりにくいと思っています。
var updateItem = CreateItem();
updateItem.ForEach(x => {/*処理*/});複数系とすれば変数名、メソッド名からリストであることが分かると思います。
var updateItems = CreateItems();
updateItems.ForEach(updateItem => {/*処理*/});リストということが伝わればよいため、下記のような命名でも良いと思います。
var updateItemList = CreateItemList();
updateItemList.ForEach(updateItem => {/*処理*/});LINQに関するもの
リストを別のリストに変換する
下記のようにあるリストから別のリストを作成する処理をよく見ます。
var aClasses = CreateAClasses();
var bClasses = new List<BClass>();
foreach (var aClass in aClasses)
{
bClass.Add(new BClass(aClass.AProperty));
}
LINQを使用すれば下記のように簡潔に書けて、読む人は直感的にわかりやすいと思います。
var aClasses = CreateAClasses();
var bClasses = aClasses.Select(aClass => new BClass(aClass.AProperty)).ToList();似た例でListのなかのListを抽出する場合は、SelectManyを使用すれば取得できます。
// {{"test1","test2"},{"test3","test4"}}
var listInList = new List<List<string>>()
{
new List<string>() {"test1","test2"},
new List<string>() {"test3","test4"}
};
// {"test1","test2","test3","test4"}
var flatList = listInList.SelectMany(x => x).ToList();リストに存在するか判断する
下記のようなリストに対象が存在するか判断する処理をよく見ます。
var aClasses = CreateAClasses();
var exists = false;
foreach (var aClass in aClasses)
{
if (string.IsNullOrEmpty(aClass.AProperty))
{
exists = true;
break;
}
}LINQを使用すれば下記のように簡潔に書けて、読む側も直感的にわかりやすいと思います。
var aClasses = CreateAClasses();
var exists = aClasses.Any(aClass => string.IsNullOrEmpty(aClass.AProperty));
ループ処理中に条件分岐がある
リストに存在するか判断すると似た形で、下記のようにループ処理を行い、条件によっては別のリストに加えるというものがあります。
var aClasses = CreateAClasses();
var bClasses = new List<BClass>();
foreach (var aClass in aClasses)
{
if (string.IsNullOrEmpty(aClass.AProperty))
{
continue;
}
bClass.Add(new BClass(aClass.AProperty));
}LINQを使用すれば下記のように簡潔に書けて、読む側も直感的にわかりやすいと思います。
var aClasses = CreateAClasses();
var bClasses = aClasses
.Where(aClass => !string.IsNullOrEmpty(aClass.AProperty))
.Select(aClass => new BClass(aClass.AProperty))
.ToList();条件分岐に関するもの
boolの評価式
下記のように冗長な書き方をよく見ます。
bool isUpdatable = isUpdatable();
if(isUpdatable == true)
{
// 処理
}boolであれば評価式がいらないので下記のような記載でもわかるかと思います。
if(isUpdatable())
{
// 処理
}ただし、null許容型の場合は注意が必要です。
null許容型の場合は評価式でtrue、falseを明記する必要があります。一般的にはnullはfalseと同義で扱われると思いますが要注意です。
（そもそもboolでnull許容型にするべきではないと思います。）
bool? isUpdatable = isUpdatable();
if(isUpdatable == true)
{
// 処理
}if(isUpdatable() == true)
{
// 処理
}否定の否定
判定に使用する変数名（メソッド名）が否定系の名称で、if文の評価式も否定となっているものを見ることがあります。
if(!notUpdate)
{
// 処理
}否定の否定は直感的にわかりにくいため、変数名やメソッドの戻り値を反転する等、公定で書く方が良いと思います。
if(isUpdatable)
{
// 処理
}一連の処理で、条件分岐が多発するもの
下記のような一連の処理で条件分岐が多発するものをよく見ます。
var item = CteateItem();
if(item.Status == "A" || item.Status == "B")
{
// 処理1
}
if(item.Status == "B")
{
// 処理2
}
if(item.Status == "A" || item.Status == "C")
{
// 処理3
}
// 以降同じような処理が続くこのような場合は、抽象クラスやインターフェースを利用し、処理するクラスを分けた方が良いと思います。
インターフェースを利用して、切り出して処理を呼び出す場合のイメージを記載します。
public interface Executer
{
void Execute();
}
public class DefaultExecuter : Executer
{
public void Execute() {}
}
public class AExecuter : Executer
{
public void Execute()
{
// 処理1
// 処理3
}
}
public class BExecuter : Executer
{
public void Execute()
{
// 処理1
// 処理2
}
}
public class CExecuter : Executer
{
public void Execute()
{
// 処理3
}
}
public static class ExecuterCreator
{
public static Executer Create(string status)
{
if (status == "A")
{
return new AExecuter();
}
if (status == "B")
{
return new BExecuter();
}
if (status == "C")
{
return new CExecuter();
}
return new DefaultExecuter();
}
}CreateExecuterメソッドで、対象のクラスのインスタンスを作成して実行するイメージになります。
var item = CteateItem();
var executer = ExecuterCreator.Create(item.Status);
executer.Execute();さいごに
一部をピックアップして記載し、私の考えを紹介しました。
実際のコードは量が多かったり、変数のスコープが広かったりするため、さらに読むのが難しいと感じます。
個人の感覚による部分もありますが、見る側の視点でわかりやすいコードを書くようにできれば良いなと思います。