既存のコードを変更する時の心得 ~ どこを見ればいいかの勘所 ~

既存のコードを変更する時の心得 ~ どこを見ればいいかの勘所 ~

既存のコードを読み解くにあたって注意して読むべきポイント、読み進め方などをご紹介しています。対応する演習用の資料も含めたリポジトリが https://gitlab.com/clear-code/presentation-lets-modify-existing-code にあります。

19d304c788f07af59ec7ce48f0e0d9d7?s=128

YUKI "Piro" Hiroshi

October 02, 2018
Tweet

Transcript

  1. 12.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 理想的な仕上がりの「変更」 現在は、この機能がない・こういう 問題がある

    ✓ 機能があったとしたら・問題がなか ったとしたら、当然こう書かれてい るはずだろう (元々作った人が実装していたら、 当然こう実装されていただろう) ✓
  2. 14.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 やらないとどうなる? ある関数はsnake_case、 別の関数はcamelCase

    バラバラな名前付け ✓ ✓ 同様の関数が各モジュールにある 車輪の再発明 ✓ ✓ 「とりあえず動くように」で 無理矢理繋げたモジュール スパゲッティコード化 ✓ ✓
  3. 18.
  4. 19.
  5. 22.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 コーディングスタイルの例 命名規則 ✓

    スペースの空け方、詰め方 ✓ 改行の入れ方 ✓ 「この機能は使わない」 「この書き方はしない」 ✓
  6. 23.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 命名規則1 ClassName (Pascal

    Case / Upper Camel Case) ✓ variableName (Camel Case / Lower Camel Case) ✓ variable_name (Snake Case) ✓ file-name (Kebab Case / Chain Case) ✓ CONSTANT_NAME (すべて大文 字) ✓
  7. 24.
  8. 26.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 文字列リテラルの定義の仕方 single =

    'シングルクォート?' double = "ダブルクォート?" 使い分けるのか、常にどちらかに統一 するのか
  9. 32.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 変数はvar? let? const?

    JavaScriptでは変数の定義の仕方が3 種類ある var b = true; // 広いスコープの変数 let c = ''; // 狭いスコープの変数 const a = 10; // 狭いスコープの変更不能な変数(定数)
  10. 37.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 改行の入れ方2 if (condition)

    { ... } else { // 同じ行でelse ... } else { // 改行してからelse ... }else{ // 詰めて書くかどうかの違いもある ... }
  11. 39.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 メソッド呼び出しの丸括弧 p (1..10).inject(0)

    {|sum, i| sum + i } p((1..10).inject(0) {|sum, i| sum + i }) p (1..10).inject(0) do |sum, i| sum + i end p((1..10).inject(0) do |sum, i| sum + i end) エッジケースで挙動が変わる事もある
  12. 40.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 early returnするかどうか def

    f(condition) if condition # 本来の処理 end end def f(condition) return unless condition # 本来の処理 end
  13. 49.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 「こう書く事もできる」 色々な書き方ができる≠色々な書き 方をしていい

    ✓ どの書き方でもいいので、統一する 事が大事 ✓ 迷ったら有名プロジェクトのコーデ ィングスタイルをそのまま採用 ✓
  14. 50.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 良いコードと馴染むコードは別 リーダブルコードなどで 「これは冗長」「おすすめしない」

    とされているスタイルが 採用されている場合もある ✓ 「馴染むコードを書く事」と 「よりリーダブルなコードに直す事」 は別の作業と考えよう ✓
  15. 51.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 整形や検証は自動化しよう JavaScript →

    ESLint ✓ Ruby → RuboCop ✓ ルールに則っていない部分を 自動訂正したり警告したりする
  16. 52.
  17. 53.
  18. 55.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 設計方針を読み取る ディレクトリ構成を見る ✓

    自動テストを読む ✓ コードの流れを追う 初期化処理から ✓ 「この機能を調べたい」と 決めた部分から ✓ ✓
  19. 61.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 テストの例 # test/unit/issue_test.rb

    class IssueTest < ActiveSupport::TestCase ... def test_create_minimal issue = Issue.new(:project_id => 1, :tracker_id => 1, :author_id => 3, :subject => 'test_create') assert issue.save assert_equal issue.tracker.default_status, issue.status assert issue.description.nil? assert_nil issue.estimated_hours end
  20. 65.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 モジュールの分け方の視点3 # ログインセッションを主体とするのか

    session = Session.new(user: @user, password: @password) assert success 同じプロダクトの中でも、Modelの中と Controllerの中など文脈によっても視 点が変わってくる
  21. 66.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 モジュールの粒度(責任範囲) 入力した文章を機械学習のために整 形する例

    # トークナイズだけを担うモジュールがある words = Tokenizer::Whitespace.call( "this is a list of cool words!") ... # トークンのフィルタリングだけを担うモジュールがある words = TokenFilter::Stopword.call(words)
  22. 67.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 責任範囲の分け方 1つのモジュールがどこまでの処理 を担当する?

    ✓ この機能はこちらではなくあちらで 実装する、という判断の分かれ目は どこにある? ✓ という傾向を見よう
  23. 68.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 モジュール同士の連携のさせ 方 def

    test_add_custom_stopword_path temp_stopwords = Tempfile.new('xy', "#{File.dirname(__FILE__) + "/"}") temp_stopwords << "this words fun" temp_stopwords.close temp_stopwords_path = File.dirname(temp_stopwords) temp_stopwords_name = File.basename(temp_stopwords.path) TokenFilter::Stopword.add_custom_stopword_path(temp_stopwords_path) words = Tokenizer::Whitespace.call("this is a list of cool words!") TokenFilter::Stopword.language = temp_stopwords_name # ここまで全て前準備で、以下が本当にテストしたい機能の呼び出し部 words = TokenFilter::Stopword.call(words) assert_equal %w(list cool !), words end
  24. 71.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 呼び出し元から順に流れを追う 1 #

    config/routes.rb Rails.application.routes.draw do root :to => 'welcome#index', :as => 'home' match 'login', :to => 'account#login', :as => 'signin', :via => [:get, :post]
  25. 72.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 呼び出し元から順に流れを追う 2 #

    app/controllers/account_controller.rb class AccountController < ApplicationController ... # Login request and validation def login if request.post? authenticate_user ... ……ということではなく
  26. 74.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 真:呼び出し元から順に流れを 追う2 コマンドの実装を見る

    # bin/rails APP_PATH = File.expand_path('../../config/application', __FILE__) require_relative '../config/boot' require 'rails/commands'
  27. 75.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 真:呼び出し元から順に流れを 追う3 #

    /home/vagrant/.rbenv/versions/2.5.1/lib/ruby/gems/ # 2.5.0/gems/railties-5.2.1/lib/rails/commands.rb require "rails/command" ... Rails::Command.invoke command, ARGV
  28. 76.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 真:呼び出し元から順に流れを 追う4 #

    /home/vagrant/.rbenv/versions/2.5.1/lib/ruby/gems/ # 2.5.0/gems/railties-5.2.1/lib/rails/command.rb module Rails module Command ... class << self ... def invoke(full_namespace, args = [], **config)
  29. 79.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 表示文字列でソースコードを検 索 git

    grep を使う $ git grep ユーザー名もしくはパスワードが無効です config/locales/ja.yml: notice_account_invalid_credentials: ユーザー名もしくはパスワードが無効です
  30. 80.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 「notice_account_invalid_cr edentials」でソースコードを検 索

    $ git grep notice_account_invalid_credentials app/controllers/account_controller.rb: flash.now[:error] = l(:notice_account_invalid_credentials) ...
  31. 81.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 該当箇所の前後を見る app/controllers/ account_controller.rb

    をエディタで開いてみる def invalid_credentials logger.warn "Failed login for '#{params[:username]}' from #{request.remote_ip} at #{Time.now.utc}" flash.now[:error] = l(:notice_account_invalid_credentials)
  32. 85.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 裏技 # ruby

    raise Exception.new("here!") // JavaScript throw new Error('here!') 呼び出し元を調べたい箇所で例外を発 生させると、たいていスタックトレースを 見られる
  33. 87.
  34. 88.
  35. 93.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 コメントを読まないと分からな い事情 #

    app/models/user.rb def self.valid_notification_options(user=nil) # Note that @user.membership.size would fail since AR ignores # :include association option when doing a count if user.nil? || user.memberships.length < 1
  36. 94.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 コミットログ git log

    app/models/issue.rb ✓ git log -p app/models/ issue.rb ✓ などの方法で見られるが、 漫然と読むのは現実的でない
  37. 95.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 テーマで辿る1 例:Railsのバージョンアップにあたって どういう作業をしたのか?

    git log | grep -i 'rails' -B 4 commit 9947003a1145d66fb5457075908d1b1493763528 Author: Go MAEDA <maeda@farend.jp> Date: Wed Aug 8 13:29:11 2018 +0000 Update Rails to 5.2.1. -- commit ce1c65225037622c10568b3e6955cecce7e80fd9 Author: Jean-Philippe Lang <jp_lang@yahoo.fr> Date: Sat Jun 23 05:13:29 2018 +0000 Upgrade to Rails 5.2.0 (#23630).
  38. 96.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 テーマで辿る2 $ git

    show ce1c65225037622c10568b3e6955cecc e7e80fd9 commit ce1c65225037622c10568b3e6955cecce7e80fd9 Author: Jean-Philippe Lang <jp_lang@yahoo.fr> Date: Sat Jun 23 05:13:29 2018 +0000 Upgrade to Rails 5.2.0 (#23630). git-svn-id: http://svn.redmine.org/redmine/trunk@17410 e93f8b46-1217-0410-a6f0-8f06a7374b81 diff --git a/Gemfile b/Gemfile index 64598a5..98f27f6 100644 --- a/Gemfile +++ b/Gemfile @@ -4,7 +4,7 @@ if Gem::Version.new(Bundler::VERSION) < Gem::Version.new('1.5.0') abort "Redmine requires Bundler 1.5.0 or higher (you're using #{Bundler::VERSION}).\nPlease update with 'ge end -gem "rails", "5.1.6" +gem "rails", "5.2.0" gem "coderay", "~> 1.1.1" gem "request_store", "1.0.5" gem "mime-types", "~> 3.0" diff --git a/app/models/issue.rb b/app/models/issue.rb index 91f53c1..114d962 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -1016,8 +1016,7 @@ class Issue < ActiveRecord::Base # Returns the previous assignee whenever we're before the save
  39. 97.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 行ごとの変更履歴から辿る1 git blame

    app/models/user.rb 027bf938 app/models/user.rb (Jean-Philippe Lang 2007-03-12 17:59:02 +0000 17) 027bf938 app/models/user.rb (Jean-Philippe Lang 2007-03-12 17:59:02 +0000 18) require "digest/sha1" 027bf938 app/models/user.rb (Jean-Philippe Lang 2007-03-12 17:59:02 +0000 19) 77074571 app/models/user.rb (Jean-Philippe Lang 2009-09-12 08:36:46 +0000 20) class User < Principal a4d7a99c app/models/user.rb (Jean-Philippe Lang 2010-12-12 13:19:07 +0000 21) include Redmine::SafeAttributes e86f9711 app/models/user.rb (Toshi MARUYAMA 2011-08-21 01:57:25 +0000 22) 5a1fcf82 app/models/user.rb (Jean-Philippe Lang 2011-11-26 17:37:20 +0000 23) # Different ways of displaying/sortin 79f92a67 app/models/user.rb (Jean-Philippe Lang 2008-01-25 10:31:06 +0000 24) USER_FORMATS = { de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 25) :firstname_lastname => { de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 26) :string => '#{firstname} #{last de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 27) :order => %w(firstname lastname de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 28) :setting_order => 1 de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 29) }, 399223da app/models/user.rb (Jean-Philippe Lang 2012-10-30 08:40:12 +0000 30) :firstname_lastinitial => { 399223da app/models/user.rb (Jean-Philippe Lang 2012-10-30 08:40:12 +0000 31) :string => '#{firstname} #{last 399223da app/models/user.rb (Jean-Philippe Lang 2012-10-30 08:40:12 +0000 32) :order => %w(firstname lastname 399223da app/models/user.rb (Jean-Philippe Lang 2012-10-30 08:40:12 +0000 33) :setting_order => 2 399223da app/models/user.rb (Jean-Philippe Lang 2012-10-30 08:40:12 +0000 34) }, 6e6c6fac app/models/user.rb (Jean-Philippe Lang 2014-01-24 09:23:11 +0000 35) :firstinitial_lastname => { 6e6c6fac app/models/user.rb (Jean-Philippe Lang 2014-01-24 09:23:11 +0000 36) :string => '#{firstname.to_s.gs 6e6c6fac app/models/user.rb (Jean-Philippe Lang 2014-01-24 09:23:11 +0000 37) :order => %w(firstname lastname 6e6c6fac app/models/user.rb (Jean-Philippe Lang 2014-01-24 09:23:11 +0000 38) :setting_order => 2 6e6c6fac app/models/user.rb (Jean-Philippe Lang 2014-01-24 09:23:11 +0000 39) }, de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 40) :firstname => { de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 41) :string => '#{firstname}', de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 42) :order => %w(firstname id), 399223da app/models/user.rb (Jean-Philippe Lang 2012-10-30 08:40:12 +0000 43) :setting_order => 3 de0e0f09 app/models/user.rb (Toshi MARUYAMA 2012-10-01 07:07:49 +0000 44) },
  40. 98.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 行ごとの変更履歴から辿る2 git show

    de0e0f09 commit de0e0f09a33b20170468d9fdc67c4b5b7ac085d2 Author: Toshi MARUYAMA <marutosijp2@yahoo.co.jp> Date: Mon Oct 1 07:07:49 2012 +0000 pin user format order at setting panel (#10937) git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@10542 e93f8b46-1217-0410-a6f0-8f06a7374b81 diff --git a/app/controllers/settings_controller.rb b/app/controllers/settings_controller.rb index 3d2f9c1..e3387e4 100644 --- a/app/controllers/settings_controller.rb +++ b/app/controllers/settings_controller.rb @@ -39,7 +39,8 @@ class SettingsController < ApplicationController redirect_to :action => 'edit', :tab => params[:tab] else @options = {} - @options[:user_format] = User::USER_FORMATS.keys.collect {|f| [User.current.name(f), f.to_s] } + user_format = User::USER_FORMATS.collect{|key, value| [key, value[:setting_order]]}.sort{|a, b| a[1] <= + @options[:user_format] = user_format.collect{|v| v[0]}.collect{|f| [User.current.name(f), f.to_s]} @deliveries = ActionMailer::Base.perform_deliveries @guessed_host_and_path = request.host_with_port.dup
  41. 99.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 コミットログを活用するために は コミットメッセージに有用な情報を含

    めておかないといけない 「ユーザーがグループに所属してい ない時にログインに失敗する問題を 修正」 ✓ 「チケットの作成時に空のフィール ドを許容しないように変更」 ✓ 5W1Hのうちの「Why」
  42. 101.
  43. 102.
  44. 104.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 その前に…… 変更対象のコードを把握する 1.

    自動テストを自分で動かす 2. 変更する 3. 元から壊れていた物を「自分が壊し た」と誤解するとドツボにはまる
  45. 105.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 変更の計画を立てる 「これを実現する」というゴールを 明確にしよう

    関係のない事は可能な限り「やらない」 ✓ 別の問題によってブロックされている場合 は、そちらから片付ける ✓ ✓ 例:期日をメールで通知できるよう にする ✓
  46. 109.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 自動テストを書く(TDD) 色々な入力パターンを期待通りに 処理できるか?

    ✓ 空の値を渡すとどうなる? ✓ 数値が期待されるフィールドに文字 列を渡すとどうなる? ✓ ある基準値以上だったらこれをす る、というような処理に「基準値と等 しい値」を与えるとどうなる? ✓
  47. 112.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 テストを書きにくい=設計がま ずい モジュールの粒度が大きすぎ(責任

    範囲が広すぎ)ないか? ✓ 暗黙的に与えられるパラメータに依 存していないか?/副作用が多すぎ ないか? ✓ その初期化処理は本当に必要なの か? 関係ない物に無駄に依存して いないか? ✓
  48. 114.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 外部要因への依存部分を小さ く分けよう 時刻

    「PCの時計が示す時刻」から 「指定の文字列で表される時刻」へ ✓ ✓ メール送信 「実際のメールサーバーとのSMTPでの通 信」から 「メール送信用モジュールの呼び出し」へ ✓ ✓
  49. 115.

    既存のコードを変更する時の心得 -   〜どこを見ればいいかの勘所〜 Powered by Rabbit 2.2.0 外部要因を差し替える 時刻 テストの時は任意の時刻を引数で渡す

    ✓ ✓ メール送信 テストの時は、メール送信用モジュールの 振る舞いを真似るモジュールを使う ✓ ✓
  50. 116.
  51. 117.