かけだしのフロントエンドエンジニアがもらったレビューを振り返る

12481 ワード
レビュー フロントエンド JavaScript JavaScript テキストリンク

みなさん、初めまして。mashuです。

playgroundのアドベントカレンダー17日目です。

かけだしの僕が javascript でもらったコードレビューを振り返ってみたいなって思います。

変数名はわかりやすく

// 変更前
colorValue

// 変更後
hogeColor

全人類の悩みの種ですね。
hoge の色を指定したいときに、colorValueという命名ではなんの色か全くわかりません。
hoge の色であることを誰が見てもわかるようにhogeColorにすると直感的にわかるようになります。

javascript は作りたい機能をもとにコードを書くことが多いと思うので、その変数が何に使われるかを意識して命名したいですね。

アロー関数を使おう

// 変更前
function fuga () {
  console.log("piyo");
}

// 変更後
const fuga = () => {
  console.log("piyo");
}

javascript 初心者の僕はアロー関数という超便利なものを使ってませんでした。
アロー関数を使いたいけど書き方が function と全然違うから…って思ってる人は関数宣言から関数リテラル、function からアロー関数が2つ一気に変わっていてごっちゃになってるかもです。(僕も最初はごっちゃになってました。)
なので、まずは関数リテラルから調べてみて、次にアロー関数というふうに段階を踏むとわかりやすいと思います。

ネストを浅く

const list = ["hoge", "fuga", "hoge"];

// 変更前
if ( list.length === 3 ) {
  if ( list[0] === list[2] ) {
    console.log(list);
  }
}

// 変更後
if ( list.length !== 3 ) return;
if ( list[0] === list[2] ) {
  console.log(list);
}

ネストは2段を超えると読みづらくなります。ネストを浅くできるところは他の表現がないか、ちょっとだけ考えてみたらいいと思います。
三項演算子を使うことで、if文が1文で完結することがあるので、三項演算子も使いたいですよね。

共通部分は一回で書く

// 変更前
isHoge ? console.log("hoge") : console.log("fuga")

// 変更後
console.log( isHoge ? "hoge" : "fuga" )

三項演算子は書き方次第ではコードが読みづらくなります。
共通部分を一回で書くようにすると、読みやすく短いコードになると思います。

マジックナンバーを使わない

// 変更前
$("button").on("click", event => ({
  const buttonWidth = $(event.currentTarget).css("width")
  $(event.currentTarget).css("width", `${buttonWidth + 25}`);
});

// 変更後
const extendWidth = 25;

$("button").on("click", event => ({
  const buttonWidth = $(event.currentTarget).css("width")
  $(event.currentTarget).css("width", `${buttonWidth + extendWidth}`);
});

自分ではなんの数値か分かっていても、レビューする人はなんのための数値かわからないので、なんのための数値かわかるような変数名に代入したらいい感じになります。

ifに空文は絶対にダメ

// 変更前
if ( a === 1 && b === 1 ) {
  ;
} else if ( a === 1 ) {
  console.log("hoge");
} else if ( b === 1 ) {
  console.log("fuga");
} else {
  console.log("piyo");
}

// 変更後
if ( a === 1 && b !== 1 ) {
  console.log("hoge");
} else if ( a !== 1 && b === 1 ) {
  console.log("fuga");
} else if ( a !== 1 && b !== 1 ) {
  console.log("piyo");
}

空文は、コードを書いてるときはわかりやすいですが、レビューしている人は ん??? ってなるし、無駄なif分岐が増えるのもよくありません。
上手にif分岐をして、空文の使用を避けるといいと思います。

最後に

案件に参加させてもらって、コードは自分がわかるだけじゃダメなんだなぁと思いました。
読みやすいコード書けるように頑張ろ。

p.s. 次回リーダブルコード編でお会いしましょう

django ubuntuでのapacheの導入
tensorflow警告:Your CPU supports instructions that this TensorFlow binary was not compiled to use:AVX