【2023/1/29 ~ 2023/6/30】コードレビューで知ったことをざっくりまとめてみた part6

目次

やったこと

Rails

inactive だと、このユーザーがどんな理由でinactiveなのかが見えない

before

enum status: { active: 0, inactive: 1 }

after

enum status: { active: 0, canceled: 1, banged: 2 }

cancel_membership だと動詞形でtrait感がないので、 canceled_membership のような形にする

enumで持つ値と同じ命名にすれば良い。

User.findではなく、 current_userをreloadする

before

expect(User.find(current_user.id)).to have_attributes(

after

expect(current_user.reload).to have_attributes(

Time.zone.now かTime.current のどちらかに統一する

これはプロジェクトによって、違いそう。

beforeでexecute!を実行するのではなく、subjectを通す

before

RSpec.describe User::MaskService do
  describe "#execute!" do
    before do
      user_payment_method = build(:user_payment_method, user:)
      user_address = build(:user_address, user:)
      create(:user_default_payment_method, user:, user_payment_method:)
      create(:user_default_address, user:, user_address:)
      create(:user_provider, user:)
      create(:cart, user:)
      create(:user_token, user:)
      service = User::MaskService.new(user:)
      service.execute!
    end

    let(:user) { create(:user, :canceled, email_marketing_agreed: false) }

    it "delete user" do
      deactivated_user = user.reload
      expect(deactivated_user).to have_attributes(
        status: "canceled",
        deactivated_at: be_an_instance_of(ActiveSupport::TimeWithZone),
        email: include("deactivated"),
        first_name: "user",
        last_name: "deactivated",
        phone: nil,
        email_marketing_agreed: false,
      )
      expect(deactivated_user.user_addresses).to have_attributes(size: 0)
      expect(deactivated_user.user_payment_methods).to have_attributes(size: 0)
      expect(deactivated_user.user_providers).to have_attributes(size: 0)
      expect(deactivated_user.cart).to eq nil
      expect(deactivated_user.user_tokens).to have_attributes(size: 0)
    end
  end
end

after

RSpec.describe User::MaskService do
  describe "#execute!" do
    subject { service.execute! }

    before do
      user_payment_method = build(:user_payment_method, user:)
      user_address = build(:user_address, user:)
      create(:user_default_payment_method, user:, user_payment_method:)
      create(:user_default_address, user:, user_address:)
      create(:user_provider, user:)
      create(:cart, user:)
      create(:user_token, user:)
    end

    let(:user) { create(:user, :canceled, email_marketing_agreed: false) }
    let(:service) { User::MaskService.new(user:) }

    it "delete user" do
nn      subject
      user.reload
      expect(user).to have_attributes(
        status: "canceled",
        deactivated_at: be_an_instance_of(ActiveSupport::TimeWithZone),
        email: include("deactivated"),
        first_name: "user",
        last_name: "deactivated",
        phone: nil,
        email_marketing_agreed: false,
      )
      expect(user.user_addresses).to have_attributes(size: 0)
      expect(user.user_payment_methods).to have_attributes(size: 0)
      expect(user.user_providers).to have_attributes(size: 0)
      expect(user.cart).to eq nil
      expect(user.user_tokens).to have_attributes(size: 0)
    end
  end
end

表面上は set_cancel_reason で抽象度を高めてるんですが、内部で結局（そしてすぐさまに） cancel_membership_log という抽象度の低い メンバーに代入されている。set_cancel_reason というメソッドなので、素直に cancel_reason というメンバーに代入する

本質的に同一でないものを同一として扱わない

cancel_reason として抽象度を高めて管理しているメンバーを、そのまま cancel_membership_logs に打ち込むのは、惜しい。 本質的に同一でないものを同一として扱っているような違和感があり、実際、今後ここの一致性って崩れる可能性が十二分にある。

before

user.cancel_membership_logs.create!(cancel_reason)

after

      user.cancel_membership_logs.create!(
        reason: cancel_reason[:reason],
        description: cancel_reason[:description],
      )

sizeを直接引けるなら expect(user.user_addresses.size).to eq 0 とかの方がシンプル

sizeを直接引けるなら expect(user.user_addresses.size).to eq 0 とかの方がシンプル。既存の部分で have_attributes を使っている箇所は、jsonの中身で直接sizeにアクセスできない部分を仕方なくそう書いてるってだけなので。

before

expect(user.user_addresses).to have_attributes(size: 0)

after

expect(user.user_addresses.size).to eq 0

メールアドレス変更のフロー

reconfirmationをskipするのはマズイ。 変更後のアドレスを認証できなくなる。

[メールアドレス変更の手順]

1. ユーザーが新しいメールアドレスをフォームに入力して送信する。
2. updateアクションが実行されて、変更後のメールアドレス宛に確認メールが送信される
3. 確認メールをクリックすると、ユーザーのメールアドレスが新しいメールアドレスに更新されている。
4. 終了

パスワードリセットのフロー

• /users/password:にpostリクエストを出すと、リクエストボディのメールアドレス宛に削除メールを出せる /users/passwordをputリクエストすると、リクエストボディで指定したパスワードで、ユーザーのパスワードを更新できる

どちらもtokenなしで実行できるAPIである必要性がある また、リセットパスワードの期間がデフォルトが6時間なので、6時間のままで良い。

できる限りmodelに属する値はそのモデル名をキーに指定するようにする

modelの値でcreateとか、destoryとかがわかりやすいから。後データとして意味のあるまとまりであることがわかりやすいので。

Rspec

なんでも良いデータならfakerを使う

特定の値を指定していたら、何か意味があると思ってしまうため。

beforeは込み入った前提条件を書く時に使う。beforeで書けるものは基本letでも書ける。

raw enc だけだとよくわからないので raw_token encoded_token のように意味のわかる命名をする 省略形の命名が許されるのは、その中身が自明の場合のみ。

before

before do
  @raw, enc = Devise.token_generator.generate(User, :reset_password_token)

after

    let(:tokens) { Devise.token_generator.generate(User, :reset_password_token) }
    let(:raw_token) { tokens[0] }
    let(:encoded_token) { tokens[1] }

    before do

てか、Devise.token_generator.generateを使わなくても、send_reset_password_instructionsを使えば簡単にraw_token作れた。

    let(:user) { create(:user, email:, password: old_password) }
    let!(:default_raw_token) { user.send_reset_password_instructions }
    let(:raw_token) { default_raw_token }

上の処理って、以下の処理をletで書いた版か。

let(:raw_token) { @raw_token }

before do
  @raw_token = user.send_reset_password_instructions
end

インスタンス変数もletも本質的な違いはない

テストにおける、インスタンス変数もletも本質的な違いはない。 二つともテストに用いる「変数」を扱うために存在している。 違う点は「letは遅延参照であること」。そして「rspecではインスタンス変数よりもletを推奨していること」である。

つまり、インスタンス変数でできることは全部letで代用可能である。

フロントエンド

reset も update だと、両者の違いを利用する側でよくわからないので、片方は sendResetMail のような分かりやすい命名にする

resetPasswordとsendResetMailにした。

Sentページとかは、ページコンポーネントとして用意しない方が良い。そこにアクセスされて、メールが送信されましたが表示されるのが辛い。stateで出しわけすれば良い。

フロントにimmerを導入している理由は、いろんなところで変更があると辛いため。

専用のコンポーネントいっぱい作ると、あのコンポーネント直せばokなのに、あのコンポーネントも直さないとってなって、無駄に依存しているコンポーネント作りすぎるせいで変更箇所多くて死ぬ

何のモーダルかわからないって、何の変数なのかわからないってよく言われたな

必ず、「何の」を命名に加える。beforeの場合、引数にどんな値を入れるのか関数名だけ見て判断できない。インスタンス名.と書いたときのサジェストには関数名しか出ないからだから問題なのである。関数定義の引数を見ることで、「もしかしたら引数はemailを渡すべきなのかな、でもあっているか分からないから中身でほんとにemailで検索しているかコードを読もう」ってことになり、結果的に適切に抽象化できていない。使い手が瞬時に判断できない関数であることが分かる。このような場合、使い手が瞬時に判断できる関数名をつけた方が良い

before

def find_user_by(email)

after

def find_user_by_email(email)

〇〇Cardにするか〇〇Listにするか

基本コンポーネント名はUIを表す命名をする。〇〇Cardにするか〇〇Listにするか迷った時がよくあったので、違いをまとめる。

[カードUIの特徴]

• カードとは、関連性のある情報をまとめたものである。カード自体がボタンになり、詳しい情報への入り口としてよく使われる。
• シャドウがついていることが多い。
• 商品一覧や、記事一覧でよく使われる

[リストUIの特徴]

• リストは、同質の情報を提示し、比較・検討・検索などをするために使う。
• 同じ位置に、同じ強調度で、同じ情報が連続して並ぶため、情報比較がしやすい。
• 画像やアバターなどを置くことで、視覚的な比較も可能になる。比較のために使うので、画像は小さい方が良い。でかい画像だとページの高さがデカくなって書く情報を比較しづらくなる。
• ECサイトでお届け先の住所を一覧表示する際や、ツイッターのタイムラインにリストUIが使われている
  • ツイッターのタイムラインはボーダーで各ツイートが仕切られているだけでツイートとツイートの間に余白がない。そのため、ツイートを比較しやすいUIになっている。ある物事に対して検索した時のリスト表示のツイート一覧を見ることで、意見を比較しやすい。その結果、世間がどう思っているかが分かりやすい
  • ツイッターのタイムラインをカードで作成すると、余白があるから各ツイートの主張が強くなる。ツイートの主張が強くなるので一個一個に目を向けるようなUIになってしまい、情報の比較検討をサクッとできない

[まとめ]

• 様々な情報を概要としてユーザーに閲覧させたいなら、カードUIを使う。カードUIにするなら、コンポーネント名を〇〇Cardにする
• 比較検討のために情報を見せるなら、リストUIを使う。リストUIを使うなら、コンポーネント名を〇〇Listにする

仕事

人に聞くときは必ず複数の選択肢を提案する。リンクが必要ならリンクも貼っとく

論理を持つ。因果関係のないものを結びつけない

エラーの内容はエラーがどのように発生したかを表しているから、ちゃんと読んでトリガーを探す。

エラーが出ているだけで親切。全体の課題の切り分けをせずとも、ここでエラーが出ているんだとすぐ分かる。 全体がどのように動いているか仕組みをちゃんとわかっていると、課題の切り分けしやすいんよな。

ドキュメントを読むときは、必ず手掛かりになるキーワードを自分で想像して、そこを探してそこだけ読む

急ぎのタスクなら、終わってないなら進捗を夕方くらいに報告する

ペアプロは自分から頼む。そんなことで意外と人は怒らない

気づいてもらおうとするんではなくて、当事者からお願いしたほうが良い。

進捗過程をslackに垂れ流す。

気づいてくれたらラッキーなため。

コードレビューに対しての質問はなるべくGitHub上でする

仕様がそうだとしてもあるべき形を自分で作って、必要ないならクローズすれば良い

プルリクにはちゃんとどんなことをやったか書く。

generalでどんな機能をリリースするかを全体メンションして報告する

休みの日は思考が止められることをする

いつまでも仕事のことを考えてたら、休まらないから。

やばくなったら休職する

目上の人に関わらず嫌なことはちゃんと嫌だという。

コードレビューを復習する。色々気づきがあるから。

給与交渉は自分からやる。自分がどれくらい価値を提供してきたかを言う

意外とリモートワークを提案してもいけたので、自分が〇〇したいって思うなら、積極的に提案した方が良い。

調べすぎない。大体の情報は必要ない。記憶に残らない。公式ドキュメントがあれば何とかなる

なぜが分からないと人は動かない

コメントで文脈を無理やり分からせるようなハイコンテキストなコードはそもそも書かない。コメントは最終手段。そもそもの実装方針い問題がある可能性がある

無駄に変数定義することで、上下スクロール移動を何回もして読みづらくなる可能性がある。パスなら外部ファイルで定数として抽象化(本当に大事な情報だけを抜き出す)して保持した方がよい

上から読んで下にスムーズにいくのが理想。

コードは文脈が大事。

文脈を無視したコードが書いてあると、「これはどういう目的で書いているの？」が瞬時に分からなくて、書いた人に聞かないと背景が分からないコードになってしまっている。

なるべくスマホを触らないで、Macで完結させる

大きすぎるプルリクエストを作らない。

フロントとバックエンドは必ず分ける。フロントはできれば仮のステージング環境にあげたブランチのURLを付与したい。

コードレビューは、目の前のレビューを通すことに精一杯になっちゃう

何をしたいのか、どうあるのが理想なのかをまずは考える

上手く取り繕うなら、お土産とか何か物を渡す

最初のデイリーで今日やるタスクのことで、解像度が低いところをきく。それを聞くために事前に考えとく

今日進めるタスクでここわからんなってところを事前に考えておいて、デイリーで聞くって先方結構いいな。

あるコードを変更することで、どこのコードも変更がかかるかまで考えれるようにしたいな。

課題を解く前にまずは仕様を確認する。自分で考えてから聞く。

じゃないと相手の考えるリソースを使ってしまう。

課題を解く前にまずは方針を考えて、一致しているかを確認する。

じゃないと無駄に実装して、時間を無駄にする可能性もあるので。

仕様がこうだとしても、拡張性のある設計のが絶対良い。設計が使用に引きづられる必要はない。

中間テーブルって多対多だけではなくて、複数のテーブルに依存されるようなテーブルがある場合、そのテーブルに外部キーのカラムを作りまくるわけにはいかないので。この場合、複数のテーブルごとに中間テーブルを作った方が良い

トークンとかでこの構成が取られる

どうやったら早くできるのか。プロセスにこだわる。プロセスを考え抜く。色々試してみて、この状況ならどの手法なら早くできるかを考える

悩む前に行動せよ。そのあと考えてまた行動せよ

デザインの仕様が決まっていない状況なら、デザインにこだわりすぎない。

ただ、こうあった方が使いやすいでしょはなるべく実装した方が良い。

どこまでできているのか。これを仮にこうした場合にどうなるかをデバッグ時にやる。

点だけを学ぶのではなくて、線や面をまずは学んで、そこを知ってから点を深めていく。

全体で何をやっているか分からないと、自分が勉強する点がどのように役立つかを分からないため。あと、自分が今どこにいるのかマクロな視点が持てなくなる。

消化できないのに本を買わない。本当に読みたいって思ったやつだけ買う。読んでも印象に残らないとすぐ忘れる。血肉にならないと意味ない。

Whyを突き詰める。そうすると背景がわかってくる。突き詰められないなら、そこで終了

好きな物を見つける場合、なぜ(Why)好きなのかを繰り返して、これ以上なぜってやっても意味ないところまでやってきて、深ぼった部分の何(What)が好きなのか。を考える。

イライラしない

最近思うけど、自分の場合これらを守ることが、イライラしないコツな気がする

• タイパを意識しすぎない( 時間に対して、急ぎすぎない。大事なものほどゆっくりやるって意識を持つ)
• 日中眠くないって思うくらい、睡眠をとる
• 半年くらい無職しても生きていけるくらいの金銭的余裕を持つ

仕事していてエラーが出た時によくイライラしていたけど、今考えてみると、エラーに対してイライラしているんじゃなくて、「時間内に成果を出さないといけないのに、なんでエラー出るねん」って状況に対してイライラしている。つまり、時間が関与している。時間内に成果を出さなくて良いなら、全然イライラしないと個人的には思う。

時間が絡むもしくは時間は本質的ではないのに時間をあまりにも意識しすぎていると、人はイライラしやすいんだなと何となく思った。ただ、社会人やっていると自分の時間が減るのは必然で、その代わりに自分の時間を有意義に過ごそうと意識しすぎるから、イライラしやすいのは仕方ないことなのかなとも思うけど。 時間をなるべく意識しすぎないようにしないとな。

全ての出来事はある構造によって引き起こされているという価値観を持つ

何でもかんでも自分でやっていると思い込むと辛いので。構造をどう変えるかが大事。

一行に情報を詰め込みすぎない

スムーズに読み解くのが困難になるため。

テストはDRYじゃなくて愚直に書く。その方が仕様として見やすい。

共感するのと解決策を出すのは違う

相談持ち込まれた時に、解決策を出そうとするけど、何をすれば良いかは結局本人が一番わかっていて、共感すれば良いんよね。多分。勇気付けるような言葉を贈る方が大事。人の感情に寄り添わないロジカルってただの自分の意見の押し付けなだけだから、気をつけんとな。人って結局感情で意思決定しているから、ロジカルなのは自分自身がコード書く時だけでいんよな。問題解決もまずは人の感情により沿わないとな。

リアルが好き

メタバースとか生成AIすげーなと思うけど、やっぱその人のクセみたいなのがはっきり現れる方が感情がこもっている感じがして好きだな。

SNS疲れる

最近SNSを見ることがあったけど、やっぱSNSって本質的には必要ないなって思う。やっぱその人のこと知りたかったら話すのが手っ取り早い。SNSのタイムラインを見ていると、一度にいろんな情報が頭に入ってきて疲れる。今の自分は見なくても良いし。SaaSとかWebサービスを作った人が不特定多数の人にアプローチするためにSNSを使うのは良いと思う。異なるジャンルの少数のコミュニティを3〜４個作るって生き方のが自分にはあっている気がするな。SNSはあくまで繋がりだけ持っておくって感じ。SNSって本来しなくても良いことを惰性で自分の時間でやっているから、それなら自分が本当にやりたいことをやった方が良いと思う。リアルな繋がりやリアルな世界でやりたいことを実現したいなと思う。そこに、人の人生見てるだけに自分の人生の時間を使うんじゃなくて、自己実現や他者実現に自分の時間を割きたいなと感じた。

世の中物が多すぎるから、物をお金で消費するんじゃなくて、思い出になったりスキルとして蓄積したり人生経験になるものにお金をかけたい

直感レベルで理解してコードを書く

思考してコード書いたり設計を考えているうちはまだ作業スピードは遅いと思う。直感レベルでそれらができる状態まで行きたい

空腹時はイライラしやすい

なるべく空腹にならないようにしたい

仕事の思い出は良い思い出になりづらい。遊びの方が思い出に残る。わからんくもないな。どんな仕事をしていたかにもよるかもだけど。

仕事が終わった時に、自分の周りに誰もいなくなるっていうのが怖いなと感じるけどな。仕事の繋がりって、結局仕事があるから存在する繋がりであって、その人たちの関係性って実はめちゃくちゃ薄い。自分が自由に使える時間で、自分のやりたいこと(趣味や個人プロジェクトとか)で自分から積極的に作った人間関係の方が残りやすい。

ネットを見なくても、全体の仕組みレベルで理解してて、根底レベルで理解しているから、仕様書とかドキュメントだけを見て自分で当たり前のように理解できて、実現できるレベルにならないとあかんな。

脳みそが疲れて、どうでもいい人やどうでもいいことばっか調べてしまう

夜中に今完全に脳みそが疲れているなと思う時があります。さっさと寝ましょう。その状態でyoutubeとか見出したら永遠に抜け出せないです。あと、その状態になる前に歯磨きとか食事とか風呂とか生活習慣をやりましょう。じゃないと、頭が回らなくてそれすらやらなくなってしまいます。 夜中の頭が回ってない時のyoutubeやネットサーフィンをほど怖いものはありません。脳が疲れているので、どうでも良い情報でもずっと見てしまいます。youtubeやネットサーフィンはやるとしても朝の方が良いです。その方が自分を客観視できるし、脳の体力が残っているので、自分を制御できます。一番は仕組みで解決した方が良いです。

参考記事

【インタビュー】Pepperの父・林要さんの新会社「GROOVE X」が作るロボットとは？ - ロボスタ

【Rails】Discardを使った論理削除の実装｜Rubinistを目指す新米エンジニアのTECH BLOG

【勉強法】全体像を掴んでから細部を学ぶと効率良く学習できる｜アイ＠アウトプット練習帳【Life manager】

カードコンポーネント vs リストビュー｜きい

第15回 カードUIの向き不向き | gihyo.jp

【4】マテリアルデザイン践実 ③カードのルールと使い方 | FASTCODING BLOG

【4】マテリアルデザイン践実 ③カードのルールと使い方 | FASTCODING BLOG

場面に合ったモバイルUIデザインを！リストビューとグリッドビューの違いとは？ | UX MILK

“SNS疲れ”を感じる最大の原因は「知らなくていいことを知ってしまう」55.48%【Tier調べ】 | Web担当者Forum