content読みやすいコードを書くためにJavaScriptTypeScriptコードレビュー新人プログラマ応援プログラム設計この記事は、社内向けに書いた資料を公開の許可を得て加筆修正したものです。記事中の具体例やサンプルコードはJavaScript/TypeScriptで書かれていますが、内容自体は言語にかかわらず使えます。手伝っていただきました。随時更新していきます。モチベーションが続けば。はじめに命名について条件式について関数について変数・定数についてはじめに読みやすいコードは、コメントがなくても文章を読むように理解できます。逆に、読みにくいコードはコメントがあってもさっぱり意味がわかりません。文章を読むように理解できるコードを書くために普段気をつけていることや、コードレビューの際に重点的に見ているところをまとめました。普段から読みやすいコードを心がけている方にとっては何も目新しい物はなく、当たり前に意識しているであろうことばかりです。プログラミングを始めたばかりの方は参考にしてみてください。命名について読みやすいコードを書く第一歩は、わかりやすい名前をつけることです。わかりやすい名前とは、具体的には以下のようなものです。以下のことがわかるような名前をつけましょう。どんな情報が入っているか?ユーザーIDか?ユーザー名か?それらを含むユーザー情報オブジェクトか?「画像サイズ」は画像の縦横サイズ?画像ファイルのサイズ?入っている情報がわかれば、型も容易に推測できます。単位は何か?例えば時間の場合、日か時間か秒かミリ秒か(それともYYYYMMDDの文字列かDateクラスのインスタンスかMoment.jsのインスタンスか)曖昧になりがちなので、はっきりわかる名前にしてください。データサイズはバイトかキロバイト[KB]かメガバイト[MB]か、あるいはキビバイト[KiB]かメビバイト[MiB]か曖昧になりがちです。悪い例dataあらゆる情報はデータなので、これだけでは何が入っているのかわかりません。一時的に使う、スコープが数行程度の変数(ソート用の比較関数の引数等)であればあまり問題にはなりませんが、処理の根幹に関わる変数がこんな名前だと混乱の元です。Doe"}といったオブジェクト?)唯一許されるのは、Userクラスのインスタンスの場合のみです。fileどんなファイル情報が入っているのかわかりません(ファイル名?拡張子を除いたファイルタイトル?ファイルサイズ?)唯一許されるのは、Fileクラスのインスタンスの場合のみです。startTime時間をどんな形で持っているのかわかりません(Dateオブジェクト?YYYYMMDD形式の文字列?Unix時間?)Unix時間だとしても、単位が秒なのかミリ秒なのかわかりません。いい例userIdユーザーIDだとわかる重複のない一意の値だとわかる多分数値型じゃないかなstartTimeUnixMilliSecUnix時間(数値型)だとわかる単位はミリ秒だとわかるただし、あまり細かくやりすぎると長くなるのも事実なので(上のstartTimeUnixMilliSecも長いですね)、文脈から明らかにわかる情報は削っても構いません。(明らかにわかる情報という条件つきですが)否定形はほどほどに名前に否定形はあまり入れないほうがいいです。何か処理}何か処理するのは表示するとき?しないとき?と混乱しますね。"hide"自体が「隠す(表示しない)」という否定の意味を含んでいるので、「表示しなくなくない」でさらにややこしくなります。目的を名前にするうっかりすると、「手段」を名前につけてしまうことがあります。後から手段が変わったときに実態と異なる名前になってしまい、バグのもとです。名前には「目的」をつけましょう。みたいな名前を付けていませんか?これがメンタリズムです。以下のことがわかるような名前をつけましょう。何をしているか?引数はどう使うか?どんな情報を返すか?将来の拡張や関数の追加に対応できるか?悪い例user()ユーザーをどうにかする関数だろうと推測できますが、単一ユーザーを取得するのか、複数ユーザーを取得するのか、ユーザー数を数えるのか、ユーザー情報を更新するのか、ユーザーを削除するのか、有効なユーザーかどうかを調べるのか全くわかりません。findUsers(name)ユーザーを絞り込む関数だろうと想像できますが、関数名だけでは「名前で」絞り込むことを推測できません。将来、「ユーザーを所属グループで絞り込む関数」を追加するかもしれません。名前で絞り込む頻度が他の絞り込み方法に比べて圧倒的に高いとかでない限り、findUsers()という「特等席」を与えるべきではありません。user)ユーザーの所属グループを取得するのだろうと想像できますが、ユーザーオブジェクト(?)から呼ばれるかなんて情報は不要です。後で他の場所からも呼びたくなったら名前変えますか?関数名は何をやっているかが重要で、どこから呼ばれるかという情報は不要です。いい例countUsers()ユーザー数を取得する関数だとわかる戻り値はユーザー数(数値型)だとわかるfindUsersByName()ユーザーを名前で絞り込む関数だとわかる引数は絞り込む名前だとわかる戻り値はUserクラスのインスタンスの配列だとわかるuser.isAvailable()有効なユーザーかどうかを返すメソッドだとわかる戻り値はboolean型で、有効であればtrueだとわかるgroup.hasUser()グループにユーザーがいるかどうかを返すメソッドだとわかる戻り値はboolean型で、ユーザーがいればtrueだとわかるuser.removed()ユーザーが削除されているかどうかを返すメソッドだとわかる戻り値はboolean型で、削除されていればtrueだとわかるisRemoved()でも問題ありませんが、removed()だけでも通じます。同時にremove()メソッドもあったりすると紛らわしいので取り扱い注意。このように、関数は「何かをするもの」なので、原則として動詞を頭につけてください。「●●かどうか」を返す関数(boolean型)は、上記のように三人称単数現在形や過去分詞を使いましょう。便利な動詞たち関数名によく使われる動詞です。「その関数が何をやっているか、何を返すか」がわかりやすいものを選んでください。取得系get取得する万能感があります。searchといった、より限定的な意味の動詞を使うことを検討してください。fetch(別の場所から)取ってくるDBやネットワーク越しに何かを取得する場合に使います。find探す複数の候補の中から絞り込む場合に使います。)list複数(0以上)取得するlistを名詞として扱い、getListOfUsers()のような名前(ユーザー一覧を取得)でも間違いではないと思いますが、listUsers()(ユーザーを列挙する)のほうが簡潔に書けます。これも万能感があるので、findUser()であれば単一のユーザー情報を、findUsers()であれば複数の情報を返すという方法でもいいと思います。ような名前(ユーザーの数を取得)でも間違いではないと思いますが、countUsers()(ユーザーを数える)のほうが簡潔に書けます。(2019/01/18追記)エゴサしてたらこんなはてブコメントを見つけました。fetch,find,searchの説明は違和感。何より、howよりwhatと言うのであれば、これら全てfindでいい事になる。fetchについては、確かにHowの意味を含むので「どこかから取ってくる」と明示したい場合にのみ使うべきです。APIやDB操作のようなライブラリが該当します。find/searchについては、関数の役割次第です。dirName)、システム全体(あるいはアプリケーションが関知している領域全体)から探すならfindFilesByPattern(glob)のような名前がよさそうです。「どこから」「どうやって」という情報が必要ないなら指摘の通りfindで問題ありません。作成系create無から有を作る場合に使います。新規ユーザーを作成build材料を組み立てて「構築する」場合に使います。引数として渡した条件等からSQL文字列を構築(queryという名前はSQLなのかURLのクエリストリングなのか曖昧な場合があるので、文脈上明らかな場合にのみ使ってください)generate変換や計算等で「生成する」場合に使います。IDを生成する(コールするたびに違うIDが生成される)更新系update特定のデータを更新する場合に使います。ユーザー名を更新する↑ただの例なので、「関数じゃなくてメソッドにしろよ」というツッコミは勘弁してください...saveデータを永続化する場合に使います。ユーザー情報を(DB等に)永続化する削除系deleteデータやリソースの削除に使います。ユーザー情報を削除する一部の言語(JavaScriptやC++等)ではdeleteが予約語として存在しているので、delete()という関数名は作れません。removeデータやリソースの削除に使います。ユーザー情報を削除するdeleteとの違いは以下のとおりです。削除されたものはもはや存在しない。(一覧から)取り除く。取り除かれたものはまだどこかに存在しているかもしれない。jQuery.remove()やJavaのList.remove()はDOMツリーやリスト内から要素を削除しますが、その際に削除された要素を返します。要素は完全に消滅したわけではなく、単に「取り除かれた」だけで、まだメモリ上のどこかに存在しています。これらのメソッド名がdelete()だったらネイティブからみて違和感を覚えるかもしれません。上で、このように書きました。関数は「何かをするもの」なので、原則として動詞を頭につけてください。原則には例外がつきもので、時には動詞を使わない関数名やメソッド名をつけることがあります。JavaやJavaScriptの文字列にあるcharAt()やindexOf()などがその例です。これらは関数名と引数とつなげてフレーズにしているという特殊な命名方法です。「'a'のインデックス」こういう名前を自然に作れるようになると玄人っぽいですが、一歩間違えると意味が伝わらなかったり誤解されたりするのでご利用は計画的に。単なるプロパティや属性のgetter/setterでもわざわざ動詞をつけることはないかもしれません。}}HowではなくWhatを関数を使う側にとって必要なのは、その関数が「何をするか(What)」であって、「どうやってするか(How)」ではありません。言い換えると、内部実装の情報まで関数名に含める必要はありません。内部実装まで関数名に含めてしまうと抽象度が下がり、将来実装が変わった時に誤解されてバグの元になります。MySQLから取得}もちろん、呼び出し元が内部実装を知る必要がある場合は別です。その場合でも、ほとんどが内部的に使われる関数に限られるので、exportする必要はまずないはずです。MySQLから取得}条件式について読みにくいコードにはまず間違いなく複雑な条件式があります。プログラムの規模が大きくなったり高度なアルゴリズムを使ったりすると複雑な条件が出てくるのは避けられませんが、条件が複雑ということと条件式が複雑ということはイコールではありません。条件が複雑でもわかりやすい条件式を書くことはできます。条件式は関数化して「目的」がわかるようにする例えば、以下のような条件式は何をしたいのか即断できません。||が入り乱れているので非常にわかりづらい条件式です。ここで重要なのは、本来の目的はyearが閏年かを判定したいのであって、「yearが4で割り切れるが100で割り切れない、又は、400で割り切れるか」を判定したいのではない、ということです。「4で割り切れるが...」は手段です。手段を条件式に書くと読みづらいコードになります。ifの中には手段(どうやって判定しているか)ではなく、目的(何を判定したいのか)を書きましょう。0;}単に関数化しただけですが、単体テストもやりやすくなりますし、格段に読みやすくなったとおもいませんか?複雑な条件は分解する上のisLeapYear()は関数化して全体の見通しはよくなったものの、関数の中身はまだ読みづらいままです。条件部分が関数化してあれば読みやすくするのも簡単です。読みやすくするために、条件式を分解しましょう。分解のコツは、真偽が確定したらその時点でreturnすることです。false;}これでだいぶわかりやすく、ロジックにバグがあってもすぐに気付けるレベルになりました。関数について読みやすい関数は流れが追いやすいものです。使いやすい関数は役割がわかりやすいものです。すれば読みやすく使いやすい関数を作れるのかを見ていきましょう。神=悪いろいろな引数を取り、いろいろな役割をする関数。便利ですが、1つの名前で複数の役割があると使う側はいろいろな引数のパターンや役割を覚えなくてはいけないうえに、何よりややこしいので作るのはやめましょう。ラップ」「コピー」さらに「イベント処理」までこなしてくれる全能の神。使う側は混乱してしまいます。神の領域は人間には早すぎる。文字列の内容によって検索したり生成したり挙動が変わるので、思わぬバグを埋め込んでしまうことがあります。jQueryはとても便利なライブラリで、お世話になったことのある人も少なくはないと思いますが、こういう使いにくさもあったりします。誤解しないでいただきたいのですが、関数のオーバーロードをやめましょうと言っているわけではありません。引数に時間を受け取る関数で、DateオブジェクトでもUnix時間でもISO8601形式の文字列でも受け付けるようにするなら個人的には許容範囲だと思っています。形は違えど時間情報という本質は変わらないので、混乱もあまりないでしょうから。jQueryの例でも、「セレクタ文字列またはDOMを受け取る関数」程度であればまだわかりやすかったのではないでしょうか。