『リーダブルコード』を参考に、コードをリファクタリングしてみた。
前回の記事でコードを読みやすくするためのノウハウを学んだので、
実際にそれらのノウハウを用いて、職場で見つけた(というより無作為に選んだ)クソコードを
可能な限り読みやすくしてみようと思う。
修正前
クソコードで思い出しましたがこのサイト面白いです。
さて、まずは今回リファクタリングするコードを紹介しよう。
このコードは僕が画面を実装している際に、呼び出し先のAPIのソースを読んでいたら
見つけてしまったコードだ。
最悪な事に、設計書に記載のないシャドーコードであり、実装した人も離任している。
一応補足すると、変数名、メソッド名は大きく変更してあるし、ロジックもある程度変更している。
なので、会社のソースコードを無断掲載している訳ではないので、悪しからず。
それと、このコードはセキュリティの観点から重大な欠陥を含んでいるが、
今回はそちらに関しては目をつむる事とする。
/** * ファイルをアップロードします * @param req リクエスト * @param res レスポンス */ uploadFile(req, res) { const fileSuffix = Date.getCurrentDate(Date.FormatISO).replace(/^|0-9|/g, ''); let filePath = ''; Object.keys(req.files).forEach(key => { if (key === 'updateFile') { const path = config.get('upload_path'); if(!fs.existsSync(path)) { fs.mkdirSync(path); } const filePrefix = config.get('db_update_filePrefix'); filePath = `${path}/${filePrefix}${fileSuffix}`; fs.writeFileSync(filePath, req.files[key][0].buffer); } else if (key === 'checksumFile') { const path = config.get('upload_path'); if(!fs.existsSync(path)) { fs.mkdirSync(path); } const filePrefix = config.get('checksum_filePrefix'); filePath = `${path}/${filePrefix}${fileSuffix}`; fs.writeFileSync(filePath, req.files[key][0].buffer); } else if (key === 'importFile') { const path = config.get('upload_path'); if(!fs.existsSync(path)) { fs.mkdirSync(path); } const filePrefix = config.get('import_filePrefix'); filePath = `${path}/${filePrefix}${fileSuffix}`; fs.writeFileSync(filePath, req.files[key][0].buffer); } }); syslog.put('upload success', req); req.status(200).json('OK'); }
・・・。
ほんの数行なのに、どんな処理が行われているか理解しづらい。
真ん中に居座るif-else文が著しく可読性を下げている上に、どのブロックも殆ど同じ処理をしている。
これのせいで、処理の流れや実装意図、問題点が把握しづらくなっている。
現に、このコードには重大なバグが潜んでいる。
まず一つ、このコードが作成するファイルには拡張子がない。
そして、ファイルの作成に失敗しようが、ファイルを一つも作らなかろうが、
このコードは必ず成功ステータスを返す。
何よりコメントが悪い。何だよ「ファイルをアップロードします」って。見りゃ分かるよ！見てもわかんねえ事について書けよ！
使えそうなテクニック
このコードに対して、前回の記事で紹介したノウハウの中で使えそうなものをピックアップしてみよう。
名前に情報を詰め込む。
使えそうだ。このコードの中で「fileSuffix」という変数が登場する。
直訳すると「ファイルの接尾語」だが、これを見てもあまり直感的に理解出来ない。
なので、「fileName」(ファイル名)、「fileExtension」(拡張子)、等の分かりやすい、かつ正確な変数名に変えてしまおう。
関数の動作を具体的に記述する
「ファイルをアップロードする」と書かれているが、処理として実際に行われる事は何だろうか？
それをコメント書けば、少しはコードを理解しやすくなりそうだ。
この関数は実際には
リクエストからファイルデータを取得する。
↓
ファイルの設置先をconfigから取得する(ここには記載していないが、/tmp配下だった)。
↓
配置先にファイル名が現在日時のファイルを作成する
という処理を実施している。それを関数コメントに書こう。
実例をコメントに記述する
現在日時を取得した後に、正規表現を用いてフォーマットしている箇所がある。
コードに正規表現が入っていると、どんな値になるのか立ち止まって考えなければならなくなる。
それを防ぐために、変数値の具体的な例をコメントで記載してあげよう。
変数のスコープを縮める
すべての変数を関数の頭で宣言しなければならない、というコーディング作法は存在しない(C言語とかだとあるかも)。
例えば、「fileSuffix」が関数の頭で初期化されているが、この変数を宣言するのは
ファイルを書き込む直前でも構わない。
スコープが短ければ、頭の中で変数を覚えておく必要が無くなる。
コードの欠陥にコメントを付ける
調べたところ、このコードは別の画面からも呼び出される事が分かった。
しかし、その画面からどんな拡張子のファイルがアップロードされるのかは不明だ。
なので、一時的に拡張子を「.test」で書いといて、FIXME: コメントを付けて
後で正しい拡張子に直すように注意書きを残しておこう。
修正後
上記のコードを、先程挙げたノウハウに沿って修正した。
/** * リクエストからファイルを取得し、 * tmp配下にファイルを作成します。 * 想定するリクエスト内のファイルは一つです。 * @param req リクエスト(multipart/form-data) * @param res レスポンス */ uploadFile(req, res) { const fileBasePath = config.get('upload_path'); // アップロード先ディレクトリ let fileFullPath = ''; // ファイルのフルパス let filePrefix = ''; // ファイル名の先頭文字列 let fileExtension = ''; // 拡張子 // アップロード先のディレクトリを作成 if (!fs.existsSync(path)) { try { fs.mkdirSync(fileBasePath, { recursive: true }); } catch (e) { throw e; } } Object.keys(req.files).forEach(key => { switch (key) { // ソフトウェアアップデートファイル case 'updateFile': filePrefix = config.get('update_filePrefix'); fileExtension = '.test' // FIXME: 正しい拡張子に修正 break; // チェックサムファイル case 'checksumFile': filePrefix = config.get('checksum_filePrefix'); fileExtension = '.test' // FIXME: 正しい拡張子に修正 break; // CSVインポートファイル case 'importFile': filePrefix = config.get('import_filePrefix'); fileExtension = '.csv' break; } if (filePrefix !== '' && fileExtension !== '') { // 記号を含まないミリ秒付き日時(例: 2019112009000000) const date = Date.getCurrentDate(Date.FormatISO).replace(/^|0-9|/g, ''); const fileName = filePrefix + date + fileExtension; fileFullPath = path.join(fileBasePath, fileName); try { // /tmp配下にファイルを配置する fs.writeFileSync(fileFullPath, req.files[key][0].buffer); } catch (e) { throw e; } } }); if (fs.existsSync(fileFullPath)) { syslog.put('upload success', req); req.status(200).json('OK'); } else { throw new Error('ファイルの作成に失敗しました。'); } }
Bug Fix
- 例外処理を追加した。
- if-else文をswitch構文に書き換えて可読性を上げた。
- ファイルの拡張子を追加した。
- ファイル作成に失敗した場合はエラーレスポンスを返すように修正した。
ノウハウ適用箇所
関数のコメントに処理の具体的な動き、想定リクエストデータを記載した。
/** * リクエストからファイルを取得し、 * tmp配下にファイルを作成します。 * 想定するリクエスト内のファイルは一つです。 * @param req リクエスト(multipart/form-data) * @param res レスポンス */
使用する変数にコメントを付け、用途を説明した。
const fileBasePath = config.get('upload_path'); // アップロード先ディレクトリ let fileFullPath = ''; // ファイルのフルパス let filePrefix = ''; // ファイル名の先頭文字列 let fileExtension = ''; // 拡張子
本質ではないフォルダの作成処理を関数の先頭で片づけた。
// アップロード先のディレクトリを作成 if (!fs.existsSync(path)) { try { fs.mkdirSync(fileBasePath, { recursive: true }); } catch (e) { throw e; } }
注意コメント(FIXME: )を追加し、実施すべき修正を促した。
case 'updateFile': filePrefix = config.get('update_filePrefix'); fileExtension = '.test' // FIXME: 正しい拡張子に修正 break;
コメントに具体的な値の例を記載した。
// 記号を含まないミリ秒付き日時(例: 2019112009000000) const date = Date.getCurrentDate(Date.FormatISO).replace(/^|0-9|/g, '');
使用する直前に変数を宣言し、スコープを狭くした。
const fileName = filePrefix + date + fileExtension; fileFullPath = path.join(fileBasePath, fileName);
変数名を直感的にした。
const date = Date.getCurrentDate(Date.FormatISO).replace(/^|0-9|/g, ''); const fileName = filePrefix + date + fileExtension;
少しは読みやすくなっただろうか？
勿論、修正後も相変わらず読みづらいし、処理の流れも恐らくイケてないだろう。
しかし、修正前よりも明らかに「意図を理解しやすくなった」と同時に、「修正しやすくなった」と自負している。
後は、このコードを読んだ技術力のある人が修正してしまえばいい。
それまでに僕が出来る事は
- 理解しやすいコード + コメントを書く事
- 修正しやすいようにコードを整える事
- 自分が出来る範囲までコードを質を追求する事
以上！
