LoginSignup
7
4

More than 3 years have passed since last update.

RubyAdvent Calendar 2019Day 6
@lobin-z0x50(Wataru Maeda)in株式会社ネオジニア

現場で役に立つ！最小限の修正で仕様変更に対応する方法（Ruby）

Last updated at Posted at 2019-12-05

この記事は Ruby Advent Calendar 2019 第6日目の記事です。

時間がないのにどうしても仕様変更しなくてはならないとき、どんな考え方で対応しますか？ってお話です。

はじめに

この記事では、主に大規模Webアプリケーションの開発に於いて、新規開発ではなく修正や拡張を行うシーンを想定して、無駄な工数をなるべく削減すべく自分なりに考えて実践しているベストプラクティスを書いている。

要は、時間優先で修正にあたるとき、実際に自分が頭の中で考えていることを分かりやすく書いた、みたいな内容だ。

また、紹介するコードはRubyに特化したものになっているが、概念としては特定のプログラミング言語に依存しない、考え方そのものについて書いているつもりだ。

対象とする読者層

対象とする読者は、プログラミング経験1年以上、設計工程よりも実装工程をメインに担当していて、コーディングしてテストしての毎日に追われている若手プログラマ、オブジェクト指向の言葉は知ってるけど実際には現場で使いこなせてない感が残る、初級から中級へステップアップしようとしているエンジニア、俗に言う IT土方 、といったところか。

特に、オブジェクト指向やアーキテクチャに関する書籍を読んで何となく理解したけども、実際に仕事でそれを活用できていない、具体的な実装例を見てみたい、といった人をターゲットとしている。

この記事を読んで「なるほど、そうすればいいのか」といった気づきが得られれば幸いだ。

コードベースの紹介

では始めよう。

とある大規模システムにて、何らかの外部APIの呼び出しをラップするメソッドがあったとしよう。

ソースコードは以下のような感じだ。

the_api.rb
class TheApi
  # APIを呼び出す
  # @return [String] 正常なら 'success'、エラーなら 'fail' を返す
  def self.call(param1, param2)
    ret = 'success'

    api = ApiClient.get_instance

    # 外部APIを呼び出すような処理を想定
    api.call [param1, param2]

    # 処理結果は last_error で得られる。0 以外ならエラー。
    if api.last_error != 0
      ret = 'fail'
    end

    ret
  end
end

戻り値は String で、呼び出し側で成功か失敗かの判定を行なっている。

呼び出し側
# API呼び出し
ret = TheApi.call(product_id, base_date)
if ret == 'fail'
  # エラー発生。
  raise 'API呼び出しでエラー発生'
end

外部API自体の詳細は不明で、 ApiClient クラスはサードパーティ製であり自由に修正できない部分、ブラックボックスであるとしよう。

システム全体では、上記のようにこのメソッドを利用している箇所が 50箇所ほど あるとする。どうやらコピペして作られたようだ。

コードの善し悪しはさておき、とりあえずこの様な状態のコードベースがあり、あなたはこのシステムの仕様変更を担当することになった。

仕様変更お願いします！

チームリーダーより、仕様変更の依頼がきた。

仕様変更の概要

  • 外部APIの呼び出しがタイムアウトした場合に、リトライする様にしたい。
  • ただし、50箇所ある呼び出し元のうち、まずは1箇所だけでタイムアウトの対応をいれたい。将来的には10箇所ぐらいまで増えるかも。
  • 外部APIの仕様書によると、api.lasr_error が 3 の場合に、タイムアウトを意味すると書かれている。
  • チームリーダー「やり方は任せるから、いい感じの実装を頼むよ。でも時間は余りない。 今日中に頼む 」

なるほど、なるほど。
要件自体は簡単なハナシだ。

以下のようなイメージだ。

修正イメージ
class TheApi
  # APIを呼び出す
  # @return [String] 正常なら 'success'、エラーなら 'fail'、
  #                  タイムアウトなら 'timeout' を返す
  def self.call(param1, param2)

    # 省略

    if api.last_error == 3
      ret = 'timeout'
    elsif api.last_error != 0
      ret = 'fail'
    end

    ret
  end
end

# 呼び出し側
ret = TheApi.call(product_id, base_date)
#if ret == 'fail'  # 判定方法を変更
if ret != 'success'
  # エラー発生。

  if ret == 'timeout'
    # TODO: タイムアウトなのでリトライする！
  end
  raise 'API呼び出しでエラー発生'
end

よっしゃ、イッチョウあがり！！

いや、待て待て。

このメソッドの呼び出し箇所は他にもあと 49箇所 あるぞ。

現状では、タイムアウトだろうと他のエラーだろうと全部 'fail' を返している。

呼び出し側では ret == 'fail' で判定しているため、タイムアウト時に 'timeout' を返してしまうと、エラーとして処理されなくなくなってしまう（以下の部分）。

[再掲]呼び出し側
# API呼び出し
ret = TheApi.call(product_id, base_date)
if ret == 'fail'  # 戻り値を変更するとここを全部変更しないといけない 
  # エラー発生。
  raise 'API呼び出しでエラー発生'
end

さて、どうしたら良いだろうか。

ちょっと考えてみてほしい。

まぁ、正攻法でいくなら、戻り値を String ではなく適切なクラスに変更するべきだろうな。
処理の結果をちゃんと表現できるようなクラスを定義してそのインスタンスを返す、とか。

でも 「時間は余りない、今日中に頼む」 てことなので、そこまでやる時間がなさそうだ。
戻り値を変更してしまうと、50箇所を全て変更しないといけない。
とーぜん、変更した箇所は全てテスト（動作確認）しないといけない。

これ、アカンやつやん。。。

おっと書き忘れていたが、 現在の時刻はなんと夜の9時 だ。

働き方改革とかゆーやつは一体何やったんや。笑

やばい、今日は帰れないかも！

こんな時間に急ぎのタスクを振ってくるなんて、エゲツない話だと愚痴の一つでも言いたくなるが、他のメンバーも全員残ってるし作業を振られているようだ。何かシステムトラブルでもあったのだろうか。

とにかく今は解決策を考えよう。

文句を言うのは任された仕事をやり遂げてからだ。

修正案1 グローバル変数を使う

メソッドの呼び出し箇所を変更せずに対応する方法としてまず思いつくのは、引数や戻り値とは別のところに結果を代入する方法だ。

the_api.rb
if api.last_error == 3
  $is_timeout = true
else
  $is_timeout = false
end

こんな感じにグローバル変数としてフラグを作ってしまって、タイムアウトかどうかで真偽値をセットし、呼び出し元から参照すれば良い。

めちゃくちゃ簡単に対応できそう！よっしゃー！

ごめんなさい。

ウソです。冗談です。

実際、例えばC言語の組み込み系プログラムでもない限り、今どきのWebシステムで現実にこんな事するヤツいたら 一発レッドカード だろう1。

良い子は真似しないように。

グローバル変数とまではいかなくとも、Webならセッション変数に入れたり、せめて Thread.current を使ってスレッドセーフにしたいところだが、それでもどこからでもアクセス出来る変数という点では本質的に同じなので、極力使うべきでない。

この方法は超簡単な反面、ソフトウェアの構造を一瞬で ウンコに 複雑にしてしまう諸刃の剣であり、 最後の手段 、ぐらいに捉えておこう。

修正案2 引数追加

次はちゃんとした案です。

引数を追加し、デフォルト値を設定しておけば、呼び出し元を変更せずに済むよね。

the_api.rb
class TheApi
  # APIを呼び出す
  # @param param1
  # @param param1
  # @param detect_timeout [Boolean] タイムアウトを検出するかかどうか
  # @return 正常終了: 'success'
  #         エラー時: 'fail'
  #         タイムアウト時: 'timeout' ただし detect_timeout に true を指定した場合のみ
  def self.call(param1, param2, detect_timeout = false)
    ret = 'success'

    api = ApiClient.get_instance

    api.call [param1, param2]

    if detect_timeout && api.last_error == 3
      ret = 'timeout'
    elsif api.last_error != 0
      ret = 'fail'
    end

    ret
  end
end

簡単で分かりやすい方法だ。

この考え方は Ruby だけでなく、他のメジャーな言語でも使える一般的な方法なので、 必ず覚えておくべきイディオム と言ってもいいだろう。

しかし何らかの理由でこの方法が適用できない場合もある。

引数が既にオプション引数だらけで、追加したくない場合や、可変長引数になっていて特別な意味を持つ引数を追加できない場合だ。

可変長引数の例
# @param APIに渡すパラメータ
def call(*params)
  # 省略
end

無理やり追加できなくもないが、スマートではない。できればやりたくない。

こういったケースにもスマートに対応できる方法はないだろうか。

※ここからは call() メソッドの引数が可変長だった場合について考察していく

修正案3 ブロックを渡してエラー処理の仕方を追加する

引数を変更しない方法として、Ruby ならではのブロックを活用する方法を考えてみよう2。

the_api.rb
class TheApi
  # APIを呼び出す
  # ブロックを渡した場合は、エラーコードを引数にブロックが呼ばれる
  # @param params [Array] APIに渡すパラメータ
  # @return 正常終了: 'success'
  #         エラー時: 'fail'
  def self.call(*params)
    ret = 'success'

    api = ApiClient.get_instance

    api.call *params

    # 処理結果は last_error で得られる
    if api.last_error != 0
      ret = 'fail'
    end

    if block_given?
      # ブロックがあるときはエラーコードを引数にして呼び出す
      yield api.last_error
    end

    ret
  end
end
呼び出し側
timeout_flag = false  # タイムアウトフラグ
ret = TheApi.call(product_id, base_date) do |error_code|
  if error_code == 3
    timeout_flag = true  # フラグを立てる
  end
end

if timeout_flag
  # TODO: タイムアウトなのでリトライする
elsif ret == 'fail'
  # エラー発生。
  raise 'API呼び出しでエラー発生'
end

どうだろう。

ちょっとムリヤリ感があるかな〜

引数を追加せずに済んだが、デメリットも多い。

  • 呼び出し側ではエラー処理の方法が２つになってしまい、複雑度が上がっている
  • TheApiクラスはせっかくAPIの処理をラップしていたのに、崩れてしまっている。呼び出し元がAPIの戻り値の仕様を知っている必要があり、開放閉鎖の原則に反している
  • 今後、もっと適切な方法でブロックを使いたくなった時に、この仕組みをコールバック関数で置き換えるとなると相当な変更量になってしまう

今回は却下かな〜

もっといい方法ないですかね。おっと、そういえば、今日の終電の時間も一応調べておこう。

修正案4 戻り値に特異メソッドを付与

これも Ruby ならではの方法。
Java や C# などの静的型付け言語ではこんな発想自体あり得ない3。

the_api.rb
class TheApi
  # APIを呼び出す
  # @param params [Array] APIに渡すパラメータ
  # @return [String] 正常終了: 'success'
  #                  エラー時: 'fail' さらに is_timeout? という特異メソッドが定義されています
  def self.call(*params)
    ret = 'success'

    api = ApiClient.get_instance

    api.call params

    # 処理結果は last_error で得られる
    if api.last_error != 0
      ret = 'fail'

      # タイムアウト対応のための特異メソッドを定義
      ret.define_singleton_method(:is_timeout?) do
        api.last_error == 3
      end
    end

    ret
  end
end
呼び出し側
ret = TheApi.call(product_id, base_date)

if ret == 'fail'
  # エラー発生。
  if ret.is_timeout?
    # TODO: タイムアウトなのでリトライする
  end
  raise 'API呼び出しでエラー発生'
end

これなら、呼び出し元のエラー処理が分散することもないし、APIの仕様ラップをキープ出来ているし、引数やブロックの追加も必要ない。

変更量も少ないし、何より、呼び出し元の ret.is_timeout? が非常にシンプルで分かりやすい！4

やはり処理の結果は戻り値で戻すのが自然に感じるし、コールバックや出力用の引数で渡すのは言語仕様上 仕方ないとしてもやはり違和感が残る。

これぞ Ruby の柔軟性って感じ！5

ただし乱用は禁物。俗に言う「黒魔術的」な手法なので、慣れていない人6が見ると気持ち悪く見える。僕もあまり使う機会は少ないが、実際にはハッシュに要素を追加するぐらいの（JavaScript的な）感覚なので、そこまで弊害はなさそうに思う（初めて見たという人は感想をコメントください）。

修正案5 別のメソッドを追加する

現状の TheApi.call はそのまま置いといて、 TheApi.call_ex みたいな、別のメソッドを作ってしまって、そっちで新しい仕様に対応しようという考え方。

the_api.rb
class TheApi

  # APIを呼び出す（詳細なエラーを返すバージョン）
  # @param params [Array] APIに渡すパラメータ
  # @return [Integer] 0: 正常終了、それ以外はエラー（3: タイムアウト）
  def self.call_ex(*params)

    api = ApiClient.get_instance

    api.call *params

    # last_error をそのまま返す
    api.last_error
  end

  # APIを呼び出す（従来バージョン）
  # @param params [Array] APIに渡すパラメータ
  # @return [String] 正常終了: 'success'
  #                  エラー時: 'fail'
  def self.call(*params)

    # 残り時間によっては実装は変更しないままいくケースも考えられるが、
    # これぐらいなら頑張ってリファクタリングしてほしい。

    # 最適化するならこんな感じかな〜
    call_ex(*params) == 0 ? 'success' : 'fail'
  end

end

オブジェクト指向設計としては、個人的にはこれが一番良さそうに思うが、メリットデメリットを整理してみよう。

メリット

  • 既存のメソッドを残しつつ、新しい仕様にも対応できている。
  • 今後タイムアウト以外のエラーコードにも対応したくなった時でも変更しなくて良い。ただし、そんな事が実際あるのかどうかは現状では未確認。極端な話、もしかしたらこのAPIは廃止になるかもしれない（要は、仕様変更の方針決定に際してはそういうことまで確認すべきなのだ）。

デメリット

  • 呼び出し元でどちらのメソッドを使うべきか意識して使い分ける必要がある。考慮しないといけないことが一つ増えている。
  • 外部APIの仕様をラップしていたはずが、崩れてしまっている。すなわち、APIの結果がどうだったか判定するには外部APIの仕様を分かっている必要がある。対策としては、呼び出し側がAPIの結果を表す専用のクラスを定義（下記）し、それを戻り値にすれば更に良くなりそうたが、そこまでやる時間があるかどうか。
the_api_result.rb
# TheApi の呼び出し結果を表す
class TheApiResult
  attr_reader :error_code

  def initialize(error_code)
    @error_code = error_code
  end

  def fail?
    @error_code != 0
  end

  def timeout?
    @error_code == 3
  end

  def success?
    !fail?
  end
end

今回のように時間とのトレードオフとなると、この案は意外にも中途半端な評価になるように思う。

ガッツリとリファクタリングするなら、もっと考慮すべき事（クラスメソッドをやめて、継承可能にする、テストコードを書くなど）がたくさんあるし、時間のあるときによく考えて設計したいところ。

結論

今回の状況では、修正案4 の特異メソッドでの対応が最もお手軽で、バランスの取れた方法と判断。

これで今日もなんとか家に帰れそうだぞ！

今後もしリファクタリングするにしても、以下の様なことを確認してから検討すべきだな。

  • このAPIの利用は今後も続くのかどうか
  • 続く見込みなら、さらに仕様変更がありそうかどうか
  • ありそうなら、どんな変更を想定すべきか、どの程度か

まとめ

今回のように、（とくに大規模な開発では）純粋な設計の良し悪しだけでなく時間的な制約やチームの状況によって最適解は異なる。

本来であれば、APIの結果を 'success' 'fail' で返すなんて稚拙な設計を改めるべきであるが、現場ではそうも言ってられない7。

新規に設計する仕事よりも、既に構築されて運用中のシステムがあり、設計がイケてなくても、それを修正していくような仕事の方が圧倒的に多いのだ。

良くないコードをキレイにしたい気持ちはよく分かるが、それには時間（工数）がかかる。

つまり 費用対効果 だ。

今回の例ではほんの数行の小さなメソッドで、仕様変更の内容もたかが知れているが、実際の開発の現場ではもっと大変なことがいっぱい起こっている。そんな事ないという人は、気づいていないだけだ。本当はもっと様々な、ディスプレイの中と外の両方の状況を考慮してより良い方法を探し続けていくべきなんだ。

規模が小さいソフトウェアであっても、将来的に大きくなるのを見越しているなら考え方は同じ。

安易な考えでガチャガチャ修正しているようでは、家に帰れません。笑

ちょっとした変更であっても、複数の修正案を出して検討し（できれば誰かに相談し）、最もバランスのとれた解をチーム全体で模索することが大切だ。より良い実装で、より早く仕事が終わるのなら絶対そっちのほうがいい。

ソフトウェア開発のお仕事とは、 そういった状況判断の連続 なのだ8。

現場からは以上だ。お疲れ様でした！！

  1. 現実にはこれぐらいで出入り禁止になることはないだろうけど、要はそれぐらいヤバイ方法ってことだ。 ↩

  2. Ruby 以外の言語ではコールバック関数を引数に追加するようなイメージが近い。 ↩

  3. 僕も実際にはじめてみたときは衝撃的すぎて度肝を抜かれた。笑 ↩

  4. 冒頭の修正イメージで掲載したコードに一番近い。僕は「こんな風に書けたらいいな」という発想で解決策を模索することが多いと思う。Ruby ならそれが実現しやすい。 ↩

  5. この特異メソッドの使い方を紹介したくてこの記事を書いたのだ！ ↩

  6. 特に Java や C# などの静的型付け言語から来た人 ↩

  7. Ruby で開発しているようなハイスキルエンジニアならこんなことあり得ないのかもしれないが。 ↩

  8. 将棋や麻雀などのゲームをやっているときの思考に近いように思う。サッカーも近いのかも。僕は球技全般まったく出来ないけど。笑 ↩

7
4
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
7
4