特に意味のないデフォルト引数が保守性を低下させるので注意

機能追加の依頼がきた😮

いつものように業務をこなしていると、スケジュール管理機能にとある機能追加の依頼が来ました。
内容としては複数のスケジュールを作成する関数で祝日での場合には、祝日に該当する日にはスケジュールを作成しないようにしたいとのことです。 「お、こんなのすぐ出来るやん！」と思い、修正対象の関数を見てみると以下のように定義されていました。

def create_schedules(end_date, start_date = nil)
  :
  :
end

処理の内容としては第2引数のstart_dateから第1引数のend_dateまでの日付にスケジュールを作成するというシンプルなものです。
開始日を指定する第2引数にはデフォルト引数が設定されており、start_dateに指定がない場合には当日からend_dateまでの日付にスケジュールを作成するという仕様になっていました。

def create_schedules(end_date, start_date = nil)
    start_date = start_date.nil? ? Time.zone.now : start_date
end

発生した問題🙄

このデフォルト引数が設定されていたことで結果的に実装の工数がかなり増えてしまいました。自分としてはcreate_schedulesの第3引数に祝日の場合にスケジュールを作成しないためのフラグ変数not_created_on_public_holiday(Boolean)を追加したいなと考えていました。
しかし第2引数がデフォルト引数として定義されてしまっているため、新しく引数を追加する場合には、今回追加を検討している第3引数も同様にデフォルト引数にするか、start_date = nilの前の箇所へ引数を追加する必要があります。
not_created_on_public_holidayに対してデフォルト引数を追加するメリットがない & end_dateとstart_dateが順に並んでいた方が使用感が良いと判断したので、今回は第1引数として新たな引数を追加して以下のように関数を定義することにしました。

def create_schedules(not_created_on_public_holiday, end_date, start_date = nil)
    start_date = start_date.nil? ? Time.zone.now.to_date : start_date
end

あとはnot_created_on_public_holidayのtrue/falseに従って処理を変化させてあげればいいだけなのですが、create_schedulesを呼び出している箇所がそこそこあり、今回の引数の追加と順番の変更に多くの時間を使うことになってしまいました。
仮にcreate_schedulesが以下のように定義されていたのであれば、もっと早く修正をする事が出来ました。また祝日にスケジュールしないというデフォルト引数の設定も簡単に出来ました。

def create_schedules(start_date, end_date)
  :
end

# 祝日にデフォルトでスケジュールを作成しないver
def create_schedules(start_date, end_date, not_created_on_public_holiday = false)
  :
end

反省点✒️

呼び出し元が多かったので、not_created_on_public_holidayにデフォルト引数が適応出来れば、修正はかなり楽だったでしょう。またend_dateとstart_dateの順序が逆になっているのも気になります。引数1つの定義の仕方で、これだけ作業量が変わってくるのかと思い知らされました。
特に意味のないデフォルト引数の場合には可能な限り使用を避けるべきです。今回の場合はstart_dateとend_dateという特定の期間に対してスケジュールを作成するという仕様は当然の仕様なのでデフォルト引数を用いる必要はありません。

仮にstart_dateを当日に固定してスケジュールを作成したいのであれば、こんな感じでラップした関数を定義してあげれば良いです。

def create_schedules_today_to_end(end_date)
  start_date = Time.zone.now.to_date
  create_schedules(start_date, end_date)
end

def create_schedules(start_date, end_date)
  :
end

デフォルト引数でやりたいことの多くは、パラメーターを固定したラップ関数を定義してあげれば表現可能です。
もしくは部分適応を用いてパラメーターを固定することでも表現可能ですが、言語との相性もあるので、どちらを使用するかは相性次第かなと考えます。
少なくとも今回、自分が担当したRubyではラップ関数を定義した方が楽ですね。

const create_schedules = (start_date) => (end_date) => {
    console.log(`${start_date} ~ ${end_date}`);
};

const fixedToday = create_schedules("2021/10/08");
fixedToday("2021/10/21"); // 2021/10/08 ~ 2021/10/21

参考

リファクタリング 既存のコードを安全に改善する（第2版）

qiita.com

ifritjp.github.io

動的型付け言語(Ruby)でも関数の戻り値の型は可能な限り統一した方が良い

出会いは突然に😲

いつものように眠い目を擦りながらコードレビューをしていると以下のようなコードに遭遇しました。
(※部分的に書き換えてあるので、実際には動作していないコードです)

def fetch_companies(setting_id)
  setting = Setting.find_by(id: setting_id)
  if setting
    companies = Company.where(setting.kind)
    companies
  end
end

処理としては指定されたidのSettingデータを取得して、Settingに記録されているkindを持つCompanyの一覧を取得するという単純なものです。ActiveRecord(O/RMapper)を使ったデータベースとのやりとりを行う処理です。
動作自体には何の問題もないのですが、この関数は指定したidを持つ、Settingの値が見つかった時と見つからなかった時で返す値の型が異なります。

RubyやElixirでは明示的にreturn構文を書かなくても最終行に記述されている値が自動で返ります。現時点では以下のように値が返ります。

• Settingが見つかった時 -> Companyの配列 もしくは空配列
  • whereは1件もヒットしなかった場合に空配列を返します
• Settingが見つからなかった時 -> nil

def verification
end

res = verification
p res # nil

この関数に型アノテーションをしてあげるとすると[Company] | nilという感じになります。

何が問題なのか🤔

状態によって返る値の型が異なることが分かりましたが、これがどのような問題につながるでしょうか。
オブジェクト指向をサポートする言語でよくある例としてはfetch_companiesの戻り値に対してmapやfilterを適応したい場合です。

resp_companies = fetch_companies(6)
extract_names = resp_companies.map { |company| company.name }

このコードはSettingが存在していた場合にCompanyの配列が返ってきてくれた場合には上手く動作してくれますが、Settingが見つからなかった場合にはどうでしょうか。先ほど記述したように、その場合にはnilが返ってきます。nilにはmap関数は実装されていないので、当然ですがnil::NilClassエラーになります。

nil.map { |company| company.name }
# undefined method `map' for nil:NilClass (NoMethodError)

この問題を回避するために、fetch_companiesの呼び出し元で戻り値を精査する必要があります。

resp_companies = fetch_companies(6)
return [] if resp_companies.empty? 
extract_names = resp_companies.map { |company| company.name }

fetch_companiesを呼び出す箇所が少なければ良いのでしょうが、たくさんあったとするならば、この単調な判定を各所に記述するのは面倒です。

解決策📖

解決方法はシンプルで静的型付け言語で当たり前のように行う、関数の戻り値の型を統一してあげれば良いです。
今回の場合では空配列を返すのが適切です。これでSettingの有無に関わらず、全ての場合で空配列か、Companyの配列が返るようになります。

# fetch_companies :: number -> [Company] 
def fetch_companies(setting_id)
  setting = Setting.find_by(id: setting_id)
  return [] if setting.nil?

  Company.where(setting.kind)
end

心なしかifのネストも無くなりコードの見栄えも良くなりました。必ず配列が返るので先程の問題は発生しません。 合わせて、呼び出し元でnilが返ってくる場合を考えなくて良くなりました。やりました。関数の戻り値の型を揃えるのは良いことだらけです🎉

xn--97-273ae6a4irb6e2hsoiozc2g4b8082p.com

入門Haskellプログラミング

プログラミングの基礎 ((Computer Science Library))

ファシリテーターをまかせたら普通にみんなやってくれた

リモートワークのファシリってつらい

現在、弊社ではコロナ対策として、完全リモートワークに移行しており、よほどのことがない限りは在宅にて勤務をしております。開発業務がメインなのですが、スクラムマスターという立場上、ファシリテーターとして主体的に話す機会や、プロダクトオーナーや外部の方とMTGをする機会がそこそこあったりします。

特に毎日の朝会(デイリースクラム)では、ファシリテーターとして現在のタスクの進捗状況を確認する必要があります。定刻になり、「朝会やります〜」の一言から一日の仕事が始まります。

リモートワークに移行して早一年、この環境にも慣れてきたせいか、チームメンバーの反応が薄くなってきたなぁと感じています。 移行当初は問題が起きれば、zoomで声をかけて確認、相談といったことが当たり前でしたが、気づけばslackを使ったコミュニケーションに移行しました。これ自体は効率化を図るためであり、無駄な時間を双方使う必要がないので、素晴らしいことです。しかしながら、この温度感を朝会のような場に持ってこられるとファシリテーターとしてはつらいところがあります。

👨「進捗はどうでしょうか？」
👨‍👨‍👦‍👦「...（シーン)」
👨「何か困っていることはありますか？」
👨‍👨‍👦‍👦「...（シーン)」
👨「不明な点はありますか？」
👨‍👨‍👦‍👦「...（シーン)」

🥺

と様々な問いかけをするのですが、無反応になってしまうor特定のメンバーだけが受け答えをするという状況になってしまいました。 反応がない以上、こちら側としてはOKなのか分からないですし、返答を待つ間(返答がないかもしれませんが)の沈黙はかなりつらいものがあります。

外部メンターの方に相談してみた

上記の発生している問題を外部メンターの方に相談してみました。頂いたアドバイスを要約しました。

人が何かを決めてくれるというのは非常に楽。今、発言をする必要があるという認識をしてもらう必要がある。また、発言がしやすい雰囲気であることも大切。そのためにファシリテーターとして踏み込みすぎていないか(内容を決めてyes/noで聞いていないか)を注意してみてください。

確かに自分は先に全てを説明して、内容を決めた後に「どうでしょうか」と聞いてしまう、まさに踏み込みすぎていました。この点に関しては個人的な反省点として気をつけていかなければなりません。雰囲気作りに関しても、自分の腕の見せ所ですぐに出来るようになるものでもないでしょう。

合わせて心理的安全性の確保についてのお話も頂きました。4つの不安をどのように取り除くかが重要とのことでした。

不安①：無知だと思われることへの不安が増す
不安②：無能だと思われることへの不安が増す
不安③：邪魔をしていると思われることへの不安が増す
不安④：ネガティブだと思われることへの不安が増す

www.dodadsj.com

難しいのは今、発言しなければならないという認識をしてもらうことです。意見を求めていることを理解してもらうためにはどうすればいいのかと相談した結果、「メンバーにもファシリテーターをやってもらえばいいんじゃないかな」とアドバイスを頂きました。

ファシリテーターをやってみることで普段、OKBさんが何を求めているのか、発言してほしいのか、反応がほしいのか、提案してほしいのかが体験を通じて理解していくことが出来ると思います。大切なのはファシリテーターの視点になってみることです。人前で話す練習にもなります。

ということでファシリテーターをまかせてみた

スプリントごとの振り返り会で上記の「みんなの反応がなくて困っている」という話を正直にしてみました。一瞬、空気が重くなってしまい「やってしまったなぁ...」と思ったのですが、メンバーから「話し出していいのか分からなかった」という声が続々と上がり始めて、皆、同じようなことで悩んでいたんだなぁと気付かされました。合わせて、メンバーにファシリテーターを任せてみたいという話をしました。

出来るかどうか分からないという不安の声が多かったですが、とりあえずやってみろの精神で1人1回ずつはやってもらうことにしました。

...。

驚いたのですが、皆、普通にファシリテーターをやれていました。「進捗どうですか？」「不明点ありますか？」と自分からメンバーに確認する姿はファシリテーターそのものでした。何を不安がっていたのか正直、驚きです。また、メンバーの各times(slackの個人チャネルのtwitterのようなもの)では、

• ファシリって難しい
• 反応ないとこんな感じなのか
• 普通に出来た

など、様々な声が上がっていました。

note.com

ファシリテーターを体験してもらうことで、メンバーの視野が広がりました。結果的には朝会で問題なければ「問題なしです」という声が積極的にあがるようになり、ファシリテーターをする側としては非常に楽になりました。出来ないかもと決めつけず、お願いしてみるという事が非常に重要だと勉強になりました。

SCRUM BOOT CAMP THE BOOK【増補改訂版】 スクラムチームではじめるアジャイル開発

(これのリモートワーク版出ないかなぁ...)

【API設計】jsonを返す時はできる限りフィールドのkeyを統一してあげよう

APIの仕様書が送られてきた

外部連携させて頂く企業様から、新規に追加されるAPIの仕様書を頂きました。ユーザーが登録しているカテゴリの総数と、その内訳を取得することが出来るAPIだそうで、新たに追加されたとのことです。しかしながら、蓋を開けてみると、何とも言えない残念なレスポンスとなっていました。まずは、以下をご確認ください。

(情報は原文より書き換えてあります)

以下はユーザーが何かしらのカテゴリーを1件以上登録している場合のレスポンスになります。

{
  "total_category": 2,
  "category": [
    {
      "id": 1,
      "name": "Python"
    },
    {
      "id": 2,
      "name": "JavaScript"
    }
  ]
}

そして、こちらがユーザーが1件もカテゴリーを登録していない場合のレスポンスです。

{
  "total_category": 0
}

お分かりいただけただろうか

カテゴリーの登録があるかどうかでjsonのレスポンスのフィールドのkeyの構造に違いがあります。1件でもカテゴリーの登録がされていれば、categoryというkeyが存在して、valueにはカテゴリー情報を持つオブジェクトの配列が入っています。しかしながら、1件もカテゴリーの登録がされていなければ、categoryというkeyが存在しないのです。

発生する2つの問題

実際には2つの問題があろうとも、動くコードを記述することは出来ます。ただ、それは対応としては複雑さを生み出し、保守性と品質を低下させます。APIのレスポンスだけを修正すれば済む問題を、呼び出し元でカバーしなければならないのです。

keyが存在しない場合の挙動

例えば、フロントエンドでユーザーが登録しているカテゴリーを一覧表示しようとした場合に以下のようなコードを書きます。

const generateCategoryDOM = () => {
  const resp = getCategory(); // 架空の関数
  const category = resp.category;
  return (
    <div>
      {category.map((detail) => {
        return (
          <p>{`ID: ${detail.id}`}</p>
          <p>{`Name: ${detail.name}`}</p>
        )
      })}
    </div>
  )
}

カテゴリーの登録が1件以上ある場合には正常に動作するでしょうが、カテゴリーが1件も登録されていない場合はどうでしょうか。const category = resp.category;の評価の結果、categoryにはundefinedが束縛されてしまいます。undefinedに対してmapを呼び出すことは出来ないため、このコードはエラーを引き起こします。

const obj = { "a": 1 };
const exist = obj.a;
console.log(exist); // 1

const notExist = obj.b;
console.log(notExist); // undefined

この問題を回避するためにコードに手を加えなければなりません。無駄なコードが増えてしまいました。

const category = resp.category || [];

「なぜcategoryが存在しない場合に空配列を代入してるんだろう？」ということをコードを見る度に考える必要がある可能性があるので、このような変更はできる限りしたくありませんが、仕方ないですね😭

型定義の煩雑化

またtypescriptでレスポンスの型を定義する場合にcategory?としなければなりません。毎回、categoryが存在するかどうかを確認するコードを書かないといけなくなる可能性があります。型定義も煩雑になってしまいました。

interface CategoryResp {
    total_category: number;
    category?: CategoryDetail;
}

interface CategoryDetail {
    id: number;
    name: string;
}

あるべき姿

カテゴリーの登録がない場合にでも、categoryというフィールドを返してくれれば、上記2つの問題は発生しません。値としては空配列が適切ですね。

{
  "total_category": 0,
  "category": []
}

カテゴリーの登録がない場合にcategoryというフィールドが返ってこないというのは、感覚としては戻り値をbooleanと定義している関数から、nilが返ってくるようなものです。実際に呼び出し元では今回、紹介した問題が発生します。他にも問題はあるかもしれません。

「APIというのはプロトコル経由で呼び出せる、ただの関数だ」と考えると、戻り値の型を揃えるのはごく自然なことかと思います。プリミティブな型であれば関数の戻り値の型を統一するというのが自然だと思います(言語によっては関数の戻り値の型が不一致でエラーになっているはず👀)。
jsonであっても、可能な限りレスポンスの型を統一してあげるべきです。

APIの設計については、この書籍が大変役立っています。今回紹介したレスポンスに関するテーマは第3章で扱っていますのでご参考までに。

Web API: The Good Parts

開発者が複雑だと思う機能はユーザーには使いこなせない

プロジェクトの肥大化

自分が参加しているプロジェクトが早いもので、スタートから2年が経ちました。
ありがたいことにお客様の数は増え続けており、今でも多くのお客様に使って頂いております。初期の頃と比べると、かなり機能がリッチになりました。 元々は、「シンプルさを保っていこう！」というKISSの原則に従い、誰が見ても初見で使えるアプリを目指していました。

意味するところは、設計の単純性（簡潔性）は成功への鍵だということと、不必要な複雑性は避けるべきだ

ja.wikipedia.org

こちらの書籍でもKISSの原則が紹介されています。

Design Rule Index 要点で学ぶ、デザインの法則150

しかしながら、お客様が増えると様々な声が上がってきます。

• 〇〇という機能が欲しい！
• 既存のこの機能をもっとカスタマイズしてほしい！
• 社内システムと連携したい！

など...
気がつけば、シンプルさを売りにしていたことを多くの人が忘れて、この提案された機能、仕様の実装に取り掛かってしまいます。自分はスクラムマスターなので、「いやいや、この機能はコンセプトからずれている」だとか「仕様が複雑すぎて使いこなせないよ」という話を提言したりします。

しかしながら、声の大きなお客様(大きな企業の...)に対しては自分の発言は無力な時があるのです。

欲しいと言われた複雑な機能を作った結果

どうしてもほしい！と言われたので対応をしました。
結果的には要望元のお客様が使いこなせない、設定を代行し、どの設定の時に、どういう動作をするのかを把握できる人物がほとんどいないという状態になりました。また、他のお客様には不要な機能であるため、使って頂けず、UIを複雑化させてしまいました。

使われていない AND 複雑なため取り除きたい機能ではありますが、それでも要望元のお客様が使っている以上、気軽に取り除くことも出来ないという最悪の状態になってしまいました。この複雑な機能を保守していかなければならないのです。

今回の教訓

実装時に開発者(エンジニア)から、「こんな複雑なもの使いこなせるんですかね」という声が上がっていました。自分もそう思います。しかしながら、上からの圧に負けて「GO！」の判断をしてしまいました。

「開発者が複雑だと思う機能は当然、ユーザーにも使いこなせないのだ」ということを改めて学びました。あるべき姿、ストーリーを把握している我々が、本当に必要かどうかを判断しないといけません。言わば、最終防衛ラインです。プロジェクトを間違った方向に向かわないようにコントロールしてあげなければいけません。

スクラムでは実際に作業する人たちが見積りをする。作業の量を見積もることが、自分たちの作業について考える良い機会にもなるんだ。

SCRUM BOOT CAMP THE BOOK【増補改訂版】 スクラムチームではじめるアジャイル開発

対策法

そのためにもプロジェクトがどういう方向に向かっているのか、強みは何か、ターゲットはどの層なのかをチームで再定義すると良いというアドバイスをメンターの方から頂きました。いわゆるインセプションデッキというものだそうです。

qiita.com

インセプションデッキを用意することで、「いや、その機能はコンセプトからずれてる」など発言がしやすくなりますし、インセプションデッキに記載されている以上、妥協することも難しくなるとのことです。

スクラムマスターとしてプロジェクトが正しい方向に向かうように導くのは簡単な事ではありませんが、今回の教訓を元に認識を改めようと思いました。