はじめに
この記事は数年コードレビューをしてきた筆者からみたコーディングのアンチパターンを紹介するものです
- 主観（
のみで構成されています）が混じります - 実際に書いた人を誹謗中傷するのではなく反面教師として自分の成長の糧にしてください
- 新人教育でレビューをしていたことが多かったため新人がやりがちなあるあるパターン多めです
- 言語はとりあえず
PHPで書いておきます（実際にレビューしたのはRubyだったりJavaだったり）
アンチパターン
パターン1:
全件取得しその後にループ内でif分岐している
$users = Model_User::find('all');
foreach($users as $user){
if($user->is_valid === 1){
//処理
}
}
↓
全件取得するのではなくwhereで条件を絞ったものに対してループをかければ良い
$users = Model_User::query()->where('is_valid', true);
foreach($users as $user){
//処理
}
パターン2:
意味のないループ、意味のないif分岐
$user = new Model_User();
$params = ['name' => 'test', 'mail_address' => 'test@test.com'];
foreach($params as $key => $value){
if($key === 'name'){
$user->name = $value;
} elseif($key === 'mail_address'){
$user->mail_address = $value;
}
}
$user->save();
↓
素直に各属性に値をセットする
$user = new Model_User();
$params = ['name' => 'test', 'mail_address' => 'test@test.com'];
$user->name = $params['name'];
$user->mail_address = $params['mail_address'];
$user->save();
パターン3:
もしxxxがtrueならtrue、falseならfalseを返すメソッド
function type_1_or_2($type){
if($type === 1 || $type === 2){
return true;
} else {
return false;
}
}
↓
評価式をそのまま返す
function type_1_or_2($type){
return $type === 1 || $type === 2;
}
ちなみに
上記の例に関してはtype3,4,5と追加された時にorが無限に横に長くなっていくのが気持ち悪いので以下の方がより良いかと思います
function is_valid_type($type){
$valid_types = [1,2,3,4,5];
return in_array($type, $valid_types, true);
}
注釈: 本来であれば type値は定数やEnumにするのが良いでしょう(ここでは本質的ではないので数値にしました)
パターン4:
モデルに対して自分自身のインスタンスを渡しているメソッド
clsas Model_User{
public function set_name_test($user){
$user->name = 'test';
return $user;
}
}
$user = new Model_User();
$user = $user->set_name_test($user);
$user->save();
↓
上記はstaticメソッドなのでインスタンスメソッドに変更する
そのモデルに対して自分自身のインスタンスを引数で渡すのは何かがおかしい
clsas Model_User{
public function set_name_test(){
$this->name = 'test';
}
}
$user = new Model_User();
$user->set_name_test();
$user->save();
おわりに
いかがでしたでしょうか？
ベテランエンジニアの方なら「何当たり前のこと言ってるの？」
中堅エンジニアの方なら「あーあるある、昔やったことあるわ」
新人エンジニアの方なら「昨日書いたコードアンチパターンだったわ…明日直そ…」
とかなってたら成功かなと思います
何故ダメかを理解し、よりよいコードを書いていきましょう