現在自分の書いているコードが我ながらあまりにも酷く、テストするたびに自分で自分の首を絞めている気がしてならないので、蜘蛛の糸にすがる思いで名書と名高いリーダブルコードを読んでみました。
その忘備録。
内容
・理解しやすいコードとは(第1章)
・直ぐにでも改善できる部分
-クラス、メソッド、変数など名前のつけ方 (第2章、第3章)
-段落や見目など、全体をぱーっと見た時に関わる美しさ (第4章)
-コメントの書き方 (第5章、第6章)
・ループ、ロジックの書き方 (第7章、第8章、第9章)
・コードの再構成 (第11章、第12章、第13章)
※本ではこの後に「テストと読みやすさ」「分/時間カウンタを設計・実装する」という今までのまとめがあったけれど、今回は省略。
理解しやすいコードとは
自分ではなく赤の他人が、読むだけで何やってるか理解できるコードが理想。
(50行の読みにくいコード < 100行の読み易いコード)
柔軟性に富む、新たな機能を追加し易い。
(×新しい機能を追加することになった。処理のあちこちに修正を加えなければならない) ←今ここ
すぐにでも改善できる部分
名前のつけ方 (第2章、第3章)
・名前で情報を伝える。
例:
void stop() //汎用的すぎるから、動作に合わせてよりカラフルな単語をチョイスする。
→取り消しできない重い処理ならkill()、あとから再開できるものならpause()のような。
例:
getPage()
→downloadPage, fetchPage
例:
filter() //「選択する」「除外する」など複数の意味を持つあいまいな言葉
選択→select、除外→excludeにする
他にもrecordのような名詞と動詞どちらも含む単語も分かりにくくなってしまう。
・単位などの重要な情報を詰め込む。
例: int size → int size_mb
・フォーマットで情報を伝える
例:
クラス名:CamelCase
変数名:lower_separated
メンバ変数:offset_ //最後を_で終わらせる
こうしてみた
//before
getUserId()
//after
selectUserId()
・・・getだと抽象的過ぎてよくないことを知ったから、一番ふさわしいものを慎重に選ぶという意味でselectにしてみた。
dbから取ってくる(select)データだし。
とりあえずこれから名前つける時は、直感的に知っている単語で名付けてしまう前に、一歩立ち止まって、類義語辞典チェックすることにしよう。
段落、ブロックの書き方 (第4章)
・改行位置に一貫性を持たせる
・似たような処理を行っている部分は似たようなシルエットにする
・最初の変数の宣言など、並び順が処理に直接影響を及ぼさなくても、意味や一貫性を持たせて並ばせた方が親切。そしてその順番を他の箇所でも統一させる。
・長文を羅列しない。コードを段落に分割する。
縦のラインを意識しましょう
private function pt1(Array $record)
{
$userId = $this->pickUpUserId($record['crm_user_name']);
if (empty($userId)) {
$this->exportLog[] = "担当者名が不明です\r\n";
$this->createErrorMessages($this->exportLog);
return;
}
//metaを特定
$month = $this->lastMonth;
$lReadingMetas = $this->findTargetMetas($record, $userId, $month);
if (count($lReadingMetas) != 1) {
$this->exportLog[] = "対象のMetasが特定できませんでした\r\n";
return;
}
$targetMeta = reset($lReadingMetas);
普通に気を付けていればだいたいきれいに書けるものと思うが、ネストが増えるとこれが崩れてしまう。(私の課題)
ネストを減らす工夫は後ほど。
コメントの書き方 (第5章、第6章)
コメントとして書くべきこと
・なぜこのようなやりかたになったのか、背景事情
・コードの読み手が不可解に思うであろう箇所を、あらかじめ予測して補う
・ファイルやクラス、ブロックごとに概要を書いておくと初見の人に親切○
逆に書くべきでないこと
・コードを読めばすぐに分かるようなこと。
何かしら新しい情報をもたらさないとコメント書く意味ない。
・ひどいコードを補う補助的なコメント
コードを書き直すことに注力しましょう。
書く時の注意
・代名詞を避ける
・情報密度の多い言葉でできるだけ簡潔に保つ
↑さらっと言っているけど、特に初心者にはこれがなかなか難しい...
本を読んでいても、「え、これじゃダメなの？」と思う部分が多々ありました...
そこはまあ、先輩などにアドバイスをもらう形で...
ループとロジックの書き方 (第7章、第8章、第9章)
私にとっての一番の課題でした。
めっちゃforeach回して、if文書いて、なんならその中でまたforeachして...
最初はこれが普通なのかなーって思ってたくらい。
・ガード節を使用し、返せるものは早めにreturnしてしまうことで効率上がる。
・ネストを消すために
-失敗ケースは早めにreturnする。ループ内部であればcontinuする。
・説明変数、要約変数を利用
長い式を、それを説明する名前で詰めると、読みやすくなる
・邪魔な変数を消して、if文やforeach文のスコープをできるだけ小さくする
変数に式や値をいれず、そのまま使う方が良い時もある
特に役に立たない一時変数は使わないようにする
・その変数に書き込む回数は一回だけになるようにする
以上を気をつけて巨大化したコードを直してみた
//before(真面目)
if (count($precentMonthTargetMatters) > 1) {
//or前月分と同じ値を持つものを特定
foreach ($precentMonthTargetMatters as $matter) {
$mDetailId = $matter['LReadingMatter']['m_merchant_detail_id'];
//meta.merchandiseの子に当たる値を持つエンティティを特定
foreach ($this->merchandiseDetailList as $merchandise => $value) {
if ($value['id'] == $mDetailId) {
$getMerchandise = $value['mMerchandiseId'];
if ($presentMonthTargetMeta['LReadingMetas']['merchandise'] == $getMerchandise) {
$precentTargetMatters[] = $matter;
}
}
}
if (isset($precentTargetMatters) && count($precentTargetMatters) > 1) {
//表の商品詳細から特定する。
if (array_key_exists($record['merchandise_detail'], $this->merchandiseDetailList)) {
$targetMerchandiseDetailId = $this->merchandiseDetailList[$record['merchandise_detail']]['id'];
}
foreach ($precentTargetMatters as $matter) {
if (isset($targetMerchandiseDetailId) && $matter['LReadingMatter']['m_merchant_detail_id'] == $targetMerchandiseDetailId) {
//targetmatterが特定のものに絞れたら、それだけ残して後は消す。
$precentTargetMatters = [];
$precentTargetMatters[] = $matter;
}
}
}
}
}
//after
//matterを特定
$lReadingMatters = $this->findTargetMatters($targetMeta);
if (count($lReadingMatters) > 1) {
$this->identifyTargetMatters($lReadingMatters);
}
**略**
private function identifyTargetMatters($matters, $record)
{
//ヨミ案件idから特定
foreach ($matters as $matter) {
if ($record['matter_id'] && $matter['LReadingMatter']['id'] === $record['matter_id']) {
$targetMatter[] = $matter;
return;
}
}
//idが無ければmetaの商材カテゴリと親子関係に当たるmatterの商品詳細idを持つレコードを特定
$mDtailId = $matter['LReadingMatter']['m_merchant_detail_id'];
foreach ($matters as $matter) {
$this->selectMerchantDetailId($matter, $mDtailId);
}
}
private function selectMerchantDetailId($record, $mDtailId)
{
foreach ($this->merchandiseDetailList as $merchandise => $value) {
if ($value['id'] === $mDtailId) {
$getMerchandiseId = $value['mMerchandiseId'];
if ($record['LReadingMetas']['merchandise'] == $getMerchandiseId) {
$targetMatter[] = $matter;
}
}
}
}
一番下のprivate methodもどうかと思うけれど...
ひとまずメソッド化することで見る人に優しくなりました。
50行のコードより、100行の分かり易いコード...
コードの再構成 (第10章、第11章、第12章、第13章)
・処理の本質とは無関係な下位問題を抽出し、別の関数にしていく。
例:
public function main()
{
//読み込み先ディレクトリ指定
$path = dirname(ROOT) . DS . 'var' . DS . 'backDate' . DS . $this->lastMonth . '.csv';
$this->file = $this->importCsv($path);
**略**
}
public function importCsv($filePath)
{
$csvDate = [];
$fp = fopen($filePath, 'r');
if ($fp == false) return $this->exportLog[] = "ファイルが見つかりませんでした。\r\n";
// ロケールをja_JPにセット（これをしないとマルチバイトを読み込んだ際に文字化けするので注意）
setlocale(LC_ALL, 'ja_JP');
while (!feof($fp)) {
$line = fgetcsv($fp);
if ($line) {
// 読み込むと同時に文字コードをSJISから現在の設定値へ変換する
foreach ($line as $k => $v) $line[$k] = mb_convert_encoding($v, mb_internal_encoding(), 'ASCII,JIS,UTF-8,EUC-JP,SJIS,SJIS-WIN');
$csvDate[] = $line;
}
}
特にプロジェクト固有のコードから汎用コードを切り分けていくと、他でも使い回しの効く処理のストックが増えて今後の開発が楽になる。
例で取り上げたcsv読み込み処理なんかがそれ。
csvインポートできなくて困っている後輩がいたら、importCsv()をslackに貼り付けて送れば万事解決〇
(本人のためにはならない)
・一度に一つのタスクを行うようにすること
読みにくいコードがあれば、タスクを洗い出していく。
・身近なライブラリに親しんでおく
やりたいことが、もっと簡単に実現できるかもしれない。
その機能の実装について悩まないでーーきっと必要ないから (第13章13.1)
まとめ
粗通しでした。
この記事を読んで少しでも気になる箇所があった人は『リーダブルコード』を読んで初心に返りましょう。