やわらかテック

興味のあること。業務を通して得られた発見。個人的に試してみたことをアウトプットしています🍵

【JavaScriptサンプル有り】例外が発生した後の処理を高階関数にまかせる

これは便利

こちらの書籍で紹介されていた、リファクタリングの手法の1つで、例外が発生した後の処理って場面によって何をしたいか違うよねを叶えるためのコードです。例外が発生した(された)まま、処理を終了したい、ハンドリングして、値を返したいという異なる要望を記述することが出来ます。

const campMember = ["りん", "なでしこ"];
const campValidation = (memberName, unexpectExec) => {
    if (campMember.includes(memberName)) {
        return true
    } else {
        return unexpectExec();
    }
}
const debug = (memberName, unexpectExec = () => { throw 'メンバーではありません' }) => {
    const result = campValidation(memberName, unexpectExec);
    console.log(result);
}

実行サンプル

debug("りん", () => { return false })
# true
debug("なでしこ", () => { return false })
# true
debug("はなこ", () => { return false })
# false
debug("はなこ", () => { throw '残念ながらメンバーではありません' })

// debug("はなこ", () => { throw '残念ながらメンバーではありません' })
//                      ^
// 残念ながらメンバーではありません
// (Use `node --trace-uncaught ...` to show where the exception was thrown)

ボーイスカウト・ルールが保守開発に役立った話

コードはどんどん汚くなっていく

プロジェクトのスタート時にどれだけ入念に設計をしてコードを書き始めたとしても、プロジェクトの年数が経つにつれて、コードの状態は悪くなってきます。

  • 新機能追加
  • 既存仕様の変更
  • メンバーの入れ替わり
  • リリース日が変更出来ないため、テストを書く時間がない

などなど...
コードの状態が悪くなっていく原因は多岐にわたります。

つまり、何も考えずに開発を続けていくと、気づいた時にはコードは汚れ切った状態になっている可能性があります。この状態となってしまったコードを元の綺麗な状態に戻すのは非常に困難ですし、時間がかかります。また、新機能追加や仕様変更をしていくのも初期の状態と比べると非常に難しく、多くの時間を必要とすることでしょう。

では、プロジェクトの偉い人に相談をして「コードをリファクタングしたい」と伝えたところで、その願望が叶えられることはあるのでしょうか。残念ながら、それはほとんどありません。偉い人がエンジニア畑の出身で理解がある場合は可能性があるかもしれませんが、基本的にはリファクタングはユーザーには直接的には何の利益もないので、了承を得ることは難しいです。

どうすればいいのか

こちらの書籍を読んでいる中で、ボーイスカウト・ルールというものが紹介されていました。

xn--97-273ae6a4irb6e2hsoiozc2g4b8082p.com

(実は今だけ全て無料で公開されているので、読めちゃいます👀)

ボーイスカウトには大切なルールがあります。それは、「来た時よりも美しく」です。たとえ自分が来た時にキャンプ場が汚くなっていたとしても、そしてたとえ汚したのが自分でなかったとしても、綺麗にしてからその場を去る、というルールです。

つまり、何かコードに手を加える時に、過去に書かれたコードで以下のような条件に当てはまるのであれば、リファクタリングしてあげます。

  • 何かしら気になる箇所
  • TODOコメントが記述されている箇所
  • テストが十分に書かれていない箇所
  • 処理が複雑な箇所
  • 通化されていない箇所

などなど...

少しずつでもコードを綺麗な状態にしていければ、やがて、塵も積もれば山となるの如し、コードの状態はどんどん良くなっていきます。リファクタリングの許可を偉い人に取らずとも、普段の業務に+αでボーイスカウト・ルールを意識すれば良いのです。リファクタリングは長期的に時間を取って、行うよりも、コツコツと実行していくのが良いとこちらの書籍でも言及されていました。

少しずつであっても改善をしていくことには価値があります。キャンプの古い格言に、キャンプ場を去るときは、来たときよりもきれいにして帰ろうというのがあります。駄目なコードを見かけるたびに少しずつ改善をしていけば、やがて問題はなくなっていくでしょう。

実際にチームにボーイスカウト・ルールを取り入れてみた感想

新機能追加のPRやバグFIXを行う際に、過去に記述されたコードで気になった箇所があれば、少しで良いので、手が加えられそうであれば、修正をしていきましょう!とお願いをしました。まず「過去のコードに手を加えて良い』ということに対して、メンバーが積極的ではなかったことに気づきました。本番環境で問題なく稼働しているコードに手を加えることに、抵抗があったようです。(自分も含めて).

始めた当初は関数名や変数名の更新程度でしたが、次第に各所に存在していた重複する関数がモジュールになって共通化されたり、テストケースの追加など、リファクタリングの規模が大きくなってきました。

このまま続けていけば、現状、我々が抱えている、新機能の追加がしにくいと問題は解決されそうです🙇‍♂️

ターミナルのパイプの中にElixirの処理を挟む方法を思いついた

きっかけは突然に

弊社のインフラエンジニアの方がデプロイ作業の中でこんなようなperlの処理系をコマンドの中にパイプで渡しているのを発見しました👀

ls | tail -1 | perl -ne 'if (/release command failed/){print 1}else{print 0}'

処理の内容はともかく、パイプの中に言語の処理系が組み込めることに感動しました。perlは経験のない言語なので、このためだけに新しく言語を習得することはありませんが、自分が愛用しているElixirでも同じことが出来ないか試してみました。リスト操作はElixirEnumとパイプラインが得意とするところです。
ところが、ElixirのREALiexでは上記のようにパイプの中に組み込んで引数を渡して、文字列として受け取ったコードを評価して実行するということは出来ませんでした。

出会いは突然に

その代わりといっては何ですが、色々と調査を進める内に、文字列として渡されたElixirのコードを評価して実行することが出来るCode.eval_stringという関数を発見しました。こんな感じで文字列のコードを評価して実行して結果を得ることが出来ます。

iex(1)> code = "[1,2,3,4,5] |> Enum.map(&(&1 + 5)) |> Enum.sum()"
"[1,2,3,4,5] |> Enum.map(&(&1 + 5)) |> Enum.sum()"

iex(2)> Code.eval_string(code)
{40, []}

Code.eval_stringには引数が3つまで渡すことが可能で変数を評価出来るようになっています。今回は詳細は省きますので、ドキュメントを参照して頂ければと思います。

hexdocs.pm

ひらめきは突然に

プログラミングElixirの書籍の中でも紹介されている、コマンドライン引数をパースするサンプル(第13章)のことを唐突に思い出しました。「あれ、これもしかして、コマンドライン引数から文字列を渡せば任意のコードを実行出来るんじゃ...??」と思いつき、試してみました。

完成したコードはこちらです。

github.com

コマンドラインの評価は超がつくほどシンプルです。-cというオプションの後に受け取った値をElixirのコードとして評価するようにしています。

def parse_args(argv) do
  parse = OptionParser.parse(argv, switches: [c: :boolean], aliases: [ c: :code ])
  case parse do
    { [ code: code ], args, _ } -> { :code, code, args }
    _ -> { :error, "", [] }
  end
end

あとはここでパースしたコードをCode.eval_stringに渡すだけです。

defp eval_string({ :code, code, _ }) do
  { result, _ } = Code.eval_string(code)
  result
end

作成したコードをコマンドとして実行出来るように、escriptを使ってコードをビルドします。githubの方にはすでにビルド済みのcommand_iexというバイナリファイルをあげてあります。このファイルの名前を変更してみましたが問題なく使えることを確認しました。

# mix.exsにmain関数が記述された対象のモジュールを指定
defp escript_config do
  [ main_module: CommandIex ]
end

mix escript.build

escriptsの詳細 elixirschool.com.

この時点で以下のようなことが可能になりました。ターミナルから文字列で渡したElixirのコードが評価され実行されました。

$ ./command_iex -c '[1,2,3,4,5] |> Enum.map(&(&1+5)) |> Enum.sum()'
40

パイプに組み込む

次にやりたいのは引数をパイプ経由でコードに渡すことです。先ほどは[1,2,3,4,5]という配列として扱いたい値を文字列で渡したコードの中に直接、書き込みました。これでは、不便なので引数を動的に渡して、評価するようにしてみます。Code.eval_stringの第2引数に評価したい変数名をキーワードリストで指定します。ここでネックになるのは引数として扱い値の変数名が固定されてしまうということです。マクロ等を使えば、もっと汎用性のあるコードが書けるでしょうが、簡単のため、今回はlstという変数名で固定化してあります。

# [lst: args]を追加
defp eval_string({ :code, code, args }) do
  { result, _ } = Code.eval_string(code, [lst: args])
  result
end

これで以下のようなことが可能になりました。lstにはargsのデフォルト値の空配列([])が束縛されるため、実行の結果がエラーとならず0になっています。

$ ./command_iex -c 'Enum.map(lst, &(&1+5)) |> Enum.sum()'
0

変数も評価されるようになりました。

コマンドから引数を渡す

色々なことを試しましたが、ここはElxiirの制御外で、コードの書き方でどうにかなる問題ではありませんでした。色々と調べた結果、xargsというコマンドを使えばやりたいことが出来そうだったので、試してみました。なお、xargsとパイプ経由でcommand_iexに渡せたのはコマンドの実行結果だけでした。

$ ls | xargs ./command_iex -c 'Enum.map(lst, &(&1 <> "_test"))'
["README.md_test", "_build_test", "build.sh_test", "command_iex_test",
 "lib_test", "mix.exs_test", "test_test"]

なんということでしょう!
lsコマンドの実行結果の値を引数としてcommand_iexに渡されて、値がlstに束縛され、評価され、実行されました。まさに今回やりたかったことはこれです。試しにlsの実行結果から.mdファイルだけを抽出してみます。

$ ls | xargs ./command_iex -c 'Enum.filter(lst, &(String.contains?(&1, ".md")))'
["README.md"]

やりました!!

PATHを通してどこからでも使えるようにする

このままだと/command_iexディレクトリの直下でしか実行出来ないので、どこからでも呼び出せるように自作コマンドとして登録します。私の環境はMacだったので、こちらの記事を参考に自作コマンドciex(command line iexの略)として./command_iexを呼び出すようにしました。

wemo.tech

これでciexが呼びされるようになり、どこからでも実行出来るようになりました。
(解決出来ていないエラーが出るので、お分かりの方がいましたら、ぜひPRを...🙇‍♂️).

$ ciex
** (FunctionClauseError) no function clause matching in CommandIex.eval_string/1

    The following arguments were given to CommandIex.eval_string/1:

        # 1
        {:error, "", []}

    (command_iex 0.1.0) lib/command_iex.ex:15: CommandIex.eval_string/1
    (command_iex 0.1.0) lib/command_iex.ex:10: CommandIex.main/1
    (elixir 1.11.4) lib/kernel/cli.ex:124: anonymous fn/3 in Kernel.CLI.exec_fun/2

先程のコマンドを実行出来ることを確認しました。

$ ls | xargs ciex -c 'Enum.filter(lst, &(String.contains?(&1, ".md")))'
["README.md"]

文字列として受け取れば、次のパイプにつなげることも出来ます。

$ ls | xargs ciex -c 'Enum.filter(lst, &(String.contains?(&1, ".md"))) |> Enum.at(0)'
"README.md"
$ ls | xargs ciex -c 'Enum.filter(lst, &(String.contains?(&1, ".md"))) |> Enum.atgs echo "file: "
file:  README.md

やりたいことが出来ました!

f:id:takamizawa46:20210604224157j:plain:w200

「なのでは」「ではないかと」って言い方やめてくれ

単純に不快

なんでわざわざ「なのでは」とか「ではないかと」っていう文章を送ってくるのか理解出来ません。
これが攻撃的な言い方に感じるのは自分だけでしょうか。日々、マウントをとってくるような人が使っているイメージがあります。

  • 「なのではないでしょうか」
  • 「ではないかと思います」

と表記すればいいものを...。面倒なのでしょうか。 Slackで「なのでは」「ではないかと」という返信がスレッドにされていると不快に感じてしまいます。。。

イライラするのは自分だけ?

と思って、Google先生なのでは 嫌い, なのでは うざい と調べてみたら結構ヒットしました。

www.google.com

www.google.com

みんな同じような理由で怒っていますね。これって修辞疑問って言うんですね。

~なのでは(ないでしょうか)?、という言い方を修辞疑問というそうです。
余韻を残す効果もあるのですが、おっしゃるとおり断定を巧妙に避けているともとれるため、論文などには不向きな語法とされています。

detail.chiebukuro.yahoo.co.jp

修辞疑問

修辞疑問とは、疑問文の形をしているにもかかわらず、相手に何かを訪ねているのではなく、
自分が言いたいことを相手に納得させるために反語的に疑問形で言い方です。 
肯定形の修辞疑問では否定の意味に、否定形の修辞疑問では肯定の意味になります。

なるほど。イライラしてしまうのは正常な反応な気がしてきました。

相手に何かを訪ねているのではなく、自分が言いたいことを相手に納得させるために反語的に疑問形で言い方です。

単純に使うのやめてほしい

お互いに思いやりを持って仕事をしたいですね。

Rubyのコードレビューでよく指摘すること

コードレビューはじめました

昨年の10月から勤務先企業で開発チームのリーダーになりました。元々はコードをガリガリ書くような業務をしていたのですが、ポジションが変わったため、業務の内容も変化しました。大きく変化した点としてコードをレビューしてもらう側から、コードをレビューする側になったというのがあります。実際にレビューをする側になって、気づいたことはたくさんあります。

そんな中で半年もレビューをしていると、ある程度、指摘するパターンが見えてきましたので記事にまとめておこうと思います。これが必ずしも正解というわけではなく、こういう見方もある、過去にバグが続出した際に、以下で紹介する方針をとって少しずつ改善してきたという実績を元に観点を選別しています。
自分で言うのもなんですが、レビューを行なっているコードは稼働2年以上でお客様がかなりいらっしゃる現役で稼働しているサービスのものです。

よく指摘すること

随時、更新中です。

ifがネストされすぎている(2階層以上)

プロジェクトが肥大化したり、年数が経ってくると仕様が複雑になることが多いです。結果的に設定やオプション項目が複雑化して時には複雑な条件式を作成する必要があるかもしれません。 そんな時に、条件をネストしなければいけないケースが考えられます。サンプルでコードを用意しました。分かり易さのため、煩雑さを目立つようにしました。

def set_setting_option(setting)
    mode = setting.mode
    if mode == "normal"
        if setting.options.nil?
            if setting.plan == "free"
                setting.options = { :allow: false, :details: false }
            else
                setting.options = { :allow: true, :details: false }
            end
        end
    elsif mode == "pro"
        if setting.options.nil?
            setting.options = { :allow: true, :details: true }
        end
    end
end

幾重にも条件が分岐かつネストしているため、見ただけで「うっ...」となります。また、新しくmodeが追加された時、optionsの内容が変化した際の拡張性も悪く、保守性も微妙ですね。このようなコードはレビューの対象となることが多いです。

対処としてはifを用いて処理を分岐させるだけではなく、関数を細かく作成して、やることを分散させてあげることで、それぞれのmodeの処理の追いやすさと拡張性を確保しています。また、早期returnは条件式の複雑さ、ブロックのネストを避けられるケースが多いので、使用を推奨しています。リーダブルコードでも同じような指摘がありました。

改善案(一例)

def set_setting_options(setting)
    mode = setting.mode
    case mode
    when "normal"
        set_normal_seting_options(setting)
    when "pro"
        set_pro_seting_options(setting)
    else
        raise "存在しないプランが設定されています👀"
        
    setting
end

def set_normal_seting_options(setting)
    return setting if !setting.options.nil?
    if setting.plan == "free"
        setting.options = init_options(false, false)
    else
        setting.options = init_options(true, false)
    end

    setting
end

def set_pro_seting_options(setting)
    return setting if !settings.option.nil?
    setting.options = init_options(true, true)
    
    setting
end

def init_options(allow, details)
  { :allow: allow, :details: details }
end

init_optionsに関してはオプションの項目が増えた際に、引数が増えてしんどいことになるので、ハッシュを渡すようにするなど改善の余地はまだまだありそうです。

eachが多用される

配列に対する処理として何故かeachの関数が好んで使用される傾向があることに気づきました。確かに、eachだと色んな処理が表現出来るというのは分かりますが、それぞれの関数は役割を想定して作成されているので、適材適所で最もシンプルに記述できる関数を選択してあげるのが良いです。

def filter_pro_mode(settings)
    new_settings = []
    settings.each do |setting|
        if setting.mode == "pro"
            new_settings << setting
        end
    end
    
    new_settings
end

こういったコードを本当によく見かけます。この場合は元の配列からmodeprosettingを取得したいだけなので、filterを使ってあげるのが良い選択です。

def filter_pro_mode(settings)
    settings.filter { |setting| setting.mode == "pro" }
end

非常にスッキリしました。また読み手にはfilterだから要素を絞っているというのが、関数を通して伝えることが出来ます。eachの場合には処理の中身を詳細に追う必要がありました。

関数名と実行内容が一致していない

いまだに自分もこれをやってしまう時があります。関数には何をしているかが名前から分かるように命名をしてあげるのがベストです。ただ、処理が複雑なものや、言葉で表現が難しいものがあるのも事実です。ただ、以下のような場合ではレビューの対象としています。

def check_set_mode(setting)
    mode = setting.mode
    if setting.mode != "pro" && setting.mode != "normal"
        setting.mode = "normal"
    end
    
    setting
end

この関数はcheckといいつつ、setting.modeの値を上書きしています。関数名と行なっている処理が一致していないことが分かります。この場合にはoverride_set_modeという名前が良いのではないでしょうか。

インスタンス変数が多用される

インスタンスの作成時に引数を渡して、必要な情報をインスタンス変数としてclass内に保持しておく実装は頻出で便利な書き方です。しかし、便利なインスタンス変数をよしなに使ってしまうケースがあります。インスタンス変数はインスタンス内では広いスコープを持っているため扱いには注意が必要です。

僕のおすすめとしては、インスタンス作成時に渡した値は読み取り専用で、以降の処理でインスタンス変数の値を極力、更新しないようにすることです。インスタンス変数の更新は思わぬバグを生み出します。関数の実行順序が保証されない場合は特に注意が必要です。以下のサンプルではvalidate_and_set_settingの後に別の関数が呼び出されてしまうと、@useradmin_userだった場合に挙動が変わる可能性があるので注意が必要です。

どうしてもインスタンス変数を更新したい時は専用の関数を用意する(setter)という形でレビューを通すようにしています。

class SettingAdjusetment
    def initialize(setting, request_user)
        @setting = setting
        @user = request_user
        @mode= setting.mode
        @plan = setting.plan
        @options = setting.options
    end
    
    
    def validate_and_set_setting
        return false if @plan != "free"
        return false if @options != { :allow: false, :details: false }
        
        # インスタンス変数への更新処理がある...
        if @user.admin_user?
            @mode = "pro"
            @plan = "paid"
            @options = { :allow: true, :details: true }
        end
        
    end
end

(どう書いてもインスタンス変数を避ける姿勢が身についていつので、良いサンプルを用意するのが難しいです...😅)

無理に共通化しない

複雑な条件を書いて、処理を分岐させるぐらいなら関数を分割してしまった方が早いです。過去の事例でいうと、引数で渡された値のクラスによって処理を分岐させるというコードがありました。

def update_content(content)
    if content.instance_of(String)
        content = "string :: #{content}"
    elsif content.instance_of(Number)
        content = "number :: #{content}"
    else
        content
    end

    content
end

まずinstance_ofという関数が認知の低いもので、見ただけでは何をする関数なのか分からない可能性があります。合わせて、それぞれのクラスによってどのような処理がされるのかもコードを追わないと分かりません。今回はたまたま、同じような処理がされていましたが、今後、ArrayHashの場合には、異なる処理が適応される可能性が考えられます。

このように無理に共通化をすると混乱を生み出す可能性がありますので、レビューの対象としています。改善案としては細かく関数に分割することです。

def update_string_content(content)
    "string :: #{content}"
end

def update_number_content(content)
    "number :: #{content}"
end

複雑だった判定は呼び出し側で、任意に使用する関数を選択させるようにします。そのためにも分かりやすさのために命名が重要です。

unlessを可能な限り使わない

RubyElixirではunlessを使用することが出来ますが、言語によってはサポートされていないことがあります。また、unlessで表現出来る条件式は大体がifで表現することが出来ます。ifだと条件式が長くなってしまう場合にはunlessを許可することもありますが、unlessは普段ifで慣れている分、頭が混乱するので、レビューの対象としています。

温度感としては「使うな」というわけではなく、「最終手段として使ってね」という方針にしています。

総括

ベースの考えになっているのは、「明日記憶喪失になっても読めるコード」です。プロジェクトも肥大化しており、仕様もどんどん複雑になっていきます。そんな中に今回紹介したようなコードが紛れていると、複雑、拡張性がない、把握に時間かかる、潜在的なバグがある...など様々な問題を引き起こす可能性があります。

自分の仕事は様々な問題を事前に検知して、可能な限り取り除くことです。経験則に頼る部分は多いのですが、基本的なことは書籍でも広く紹介されています。自分はリーダブルコードや関数型のパラダイムが非常に役立っているなぁと思っていますので興味があれば、手にとってみて下さい。

参考文献