はじめに

※この記事はカラオケマシン問題のネタバレを含みます。閲覧の際はお気をつけください。

毎月先輩から出していただいた課題に取り組んでいます、 mi0です。
12月はRuby初心者向けのプログラミング問題を集めてみたのカラオケマシン問題にハモリ機能を追加しました。
この記事は実装〜レビューをいただくまでの過程を纏めた備忘録です。
こうやったらもっとよくなる、などのご指摘があればコメント頂けると嬉しいです！

過去の記事はこちら！↓

登場人物

私
- 社会人２年目PG。3年目が最近の恐怖ワード。最近異様に#selectを乱用している気がして不安になってきた。
厚切りベーコン先輩
- 生ハムちゃんの先輩。何と炒めても仲良くできる。コーディングが得意。
生ハムちゃん
- 私の心の中に住んでいる妖精。ハム太●とは一切の関係がないのだ。

出題された問題

###KaraokeMachine v2 ~ ハモりモード ~

問題

カラオケマシーンに、任意の箇所でキーが変更できるような機能を追加する
更に、キーを下げる場合は、シャープではなくフラットにする改修を行う

お題：かえるのうた

例 1

C D E F |E D C |E F G A |...

↓ +2

D E F# G |F# E D |F# G A B |...
例 2

C D E F |E D C |E F G A |...

↓ -2

A♭ C D D♭ |D C A♭ |D D♭ F G |...
例 3

C D E F |E D C |E F G A |...

↓ 0 0 +2

C D E F |E D C |F# G A B |...
例 4

C D E F |E D C |E F G A |...

↓ 0 0 -2

C D E F |E D C |D D♭ F G |...

空白 = 休符
縦棒(|) = 小節の区切り

要求仕様

既存
改修
- キーを下げた場合はフラット(♭)
- キーを上げた場合はシャープ(#)
- 元のメロディが C 以外の音で始まる（予めキー変更されている）
  - 上げた場合：F# G# A# B |A# G# F# ...
  - 下げた場合：F♭ G♭ A♭ B |A♭ G♭ F♭ ...
追加
- 任意の小節でキーを上げた
- 任意の小節でキーを下げる
その他
- 入力値チェック無し

※ベースに使用するのは以下のソース

class KaraokeMachine
  DEFAULT_TONES = %w[C C# D D# E F F# G G# A A# B].freeze

  def initialize(melody)
    @melody = melody
  end

  def transpose(amount)
    return @melody if amount.zero?

    change_key(amount)
  end

  def harmonize
    ''
  end

  private

  def change_key(amount)
    @melody.scan(/\||\s|[A-G]#?/).map do |tone|
      next tone if /\||\s/.match? tone

      DEFAULT_TONES[(DEFAULT_TONES.index(tone) + amount) % 12]
    end.join
  end
end

フェーズ１　自分で考える

生ハムちゃ「とりあえず画面上で使うときのイメージを膨らませてみましょ。」

私「私のイメージでは、こんな感じかな。」

キー上げ下げを指定できるフォームがある（+1、-1など指定できる）
キーを上げ下げする範囲を指定できるフォームがある（x小節〜y小節までを指定できる）

私「これらがパラメータで飛んでくる感じ。あ、でも２小節目だけ上げたいんじゃ！みたいなときに２〜２小節って指定するのはめんどくさいなあ。１小節だけ上げたい時は2みたいな感じで１個だけ入力できたらいいな。」

生ハムちゃ「ふむふむ。キーを下げる機能は今回から増えるけどどんな風に考えてるの？」

私「それはね生ハムちゃん」

生ハムちゃ「それは？」

私「置き換え作戦を考えているのです」

生ハムちゃ「置き換え？」

私「そう、置き換え！ピアノには黒鍵があるけど、それって同じ鍵盤なのに、キーを上げる時と下げる時で呼び方が変わるんだよ。例えばF#はG♭、みたいにね。つまり黒鍵の置換さえ上手くできればキーを下げる処理は簡単にできると思うのです。」

生ハムちゃ「なるほど……？」

私「こんな感じかな」

  TONE_MAPPING = {
    'C#' => 'D♭',
    'D#' => 'E♭',
    'F#' => 'G♭',
    'G#' => 'A♭',
    'A#' => 'B♭'
  }.freeze

私「これで、メロディが既に#を含んでいる場合は♭に、逆に♭を含んでる時は#に変換しておいてからキーを変えたらいい感じにできるんじゃないかな。キーを下げる時はキーを#の音域と同じように用意した♭の音域を使って下げてあげれば良さそうじゃない？」

生ハムちゃ「なるほどね？うん、何とか実現できそう。」

私「よっし、やってみるぞぉ！」

実際に解いてみる

その１　全体のキーを下げる

私「曲全体のキー下げはchange_keyメソッド修正して実現するよ。」

ruby

   def change_key(amount)
-    @melody.scan(/\||\s|[A-G]#?/).map do |tone|
+    @melody.scan(/\||\s|[A-G]#?|[A-G]♭/).map do |tone|
       next tone if /\||\s/.match? tone
 
-      DEFAULT_TONES[(DEFAULT_TONES.index(tone) + amount) % 12]
+      change_tone(tone, amount)
     end.join
   end

私「♭がついた音を置き換えるために、正規表現の内容を変更する。で、実際に音を変換するのはchange_toneメソッドにおまかせする。」

ruby

class KaraokeMachine
   # 中略
   private

+  def change_tone(tone, amount)
+    if amount.positive?
+      tone = convert_tone(tone, amount) if flat?(tone)
+      SHARP_TONES[(SHARP_TONES.index(tone) + amount) % 12]
+    else
+      tone = convert_tone(tone, amount) if sharp?(tone)
+      FLAT_TONES[(FLAT_TONES.index(tone) + amount) % 12]
+    end
+  end

+  def sharp?(tone)
+    !tone.scan(/[A-G]#/).empty?
+  end
+
+  def flat?(tone)
+    !tone.scan(/[A-G]♭/).empty?
+  end
+
+  def convert_tone(tone, amount)
+    sharp?(tone) && amount.negative? ? +TONE_MAPPING[tone] : TONE_MAPPING.invert[tone]
+  end
end

amount.positive?がtrue
- キーを上げたい => #の音域を使う
amount.positive?がfalse
- キーを下げたい => ♭の音域を使う

私「このルールに則ってキー変更していく。で、もし#の音域を使いたいのに♭が混ざってる、なんてときはconvert_toneメソッドで先に音を置き換えておいてから、キーを変えていく。これで全音域変換できるようになったよ！！」

私「次は部分的にキーを上げ下げする機能を追加しよう！！」

その２　部分的にキーを下げる

私「今回はharmonizeメソッドを定義してハモリ機能を実現するよ〜！」

生ハムちゃ「確かキーを上げ下げする時の値は1つ、あるいは2つ受け取るようにするんだったわよね？」

私「そうだよ〜！！1小説だけキーを変更する、あるいは区間指定でキーを変更するように実装します！」

ruby

  class KaraokeMachine
  # 中略

+  def harmonize(amount, range)
+    change_point_key(amount, range)
+  end

  end

私「change_hamonizeメソッドのなかで、具体的に置換する処理を書いていくよ〜〜〜！引数のrangeは配列で受け取る想定で実装していくね！」

ruby

+  private
+  
+  def change_point_key(amount, range)
+    measures = range.size == 1 ? change_single_measure(amount, range) : change_measures(amount, range)
+    measures.join('|')
+  end

私「引数のrangeの長さが1の時は、1小節だけキーを変更するメソッドを、そうじゃない時はその範囲を指定してキーを変更してくれるメソッドを呼ぶ。」

私「まずは１小節だけキーを変えてくれるメソッド、change_single_measureから実装していこう」

ruby

+  def change_single_measure(amount, range)
+    measures = @melody.split('|')
+    tones = change_tones(measures[range[0] - 1], amount)
+    measures.delete_at(range[0] - 1)
+    measures.insert(range[0] - 1, tones)
+  end

私「小節間は|で区切られてるから、|でsplit して、作られる配列の要素番号に引数で渡された値 - 1した値、つまりキーを変換したい小節の要素番号を指定して、１小節分の文字列を取得する。」

私「それをchange_tonesメソッドの引数に渡して、キーを変更する。」

私「次に、置き換える前の小節をdelete_atで削除する。」

私「削除した箇所に置換した小節を差し込む。これで置換完了〜〜！」

私「あとは|でjoinすれば完成〜！！」

生ハムちゃ「次は範囲指定した場合の置換ね、どうするの？」

私「ふふん、引数のrangeの長さが1じゃない時に呼ぶように書いていたchange_measures`メソッドを実装して実現していきます！！よ！！」

ruby

+  private
+  
+  def change_measures(amount, range)
+    measures = @melody.split('|')
+    add_tones_to_measure(convert_tones(measures[(range[0] - 1)..(range[1] - 1)], amount), measures, range)
+    measures.slice!(range[1], range[1] - range[0] + 1)
+    measures
+  end
+
+  def add_tones_to_measure(tones, measures, range)
+    tones.each.with_index(range[0] - 1) do |measure, index|
+      measures.insert(index, measure)
+    end
+  end

私「基本のスタンスは一緒。|でsplitして、rangeで指定している範囲の小節の音を全部置き換える！」

私「それをadd_tones_to_measureメソッドの中で、元いた場所にねじ込んでいくよーーー！」

私「ねじ込んだ後に、slice!メソッドで置換前の小節を取り除く。取り除いて綺麗になった状態のmeasureを返す、と。」

私「あとは1小節だけの時と同じように|でjoinする！完成〜〜〜〜！！！」

※できたコードは以下↓

class KaraokeMachine
  SHARP_TONES = %w[C C# D D# E F F# G G# A A# B].freeze
  FLAT_TONES = %w[C D♭ D E♭ E F G♭ G A♭ A B♭ B].freeze
  TONE_MAPPING = {
    'C#' => 'D♭',
    'D#' => 'E♭',
    'F#' => 'G♭',
    'G#' => 'A♭',
    'A#' => 'B♭'
  }.freeze

  def initialize(melody)
    @melody = melody
  end

  def transpose(amount)
    return @melody if amount.zero?

    change_key(amount)
  end

  def harmonize(amount, range)
    change_point_key(amount, range)
  end

  private

  def change_key(amount)
    @melody.scan(/\||\s|[A-G]#?|[A-G]♭/).map do |tone|
      next tone if /\||\s/.match? tone

      change_tone(tone, amount)
    end.join
  end

  def change_tone(tone, amount)
    if amount.positive?
      tone = convert_tone(tone, amount) if flat?(tone)
      SHARP_TONES[(SHARP_TONES.index(tone) + amount) % 12]
    else
      tone = convert_tone(tone, amount) if sharp?(tone)
      FLAT_TONES[(FLAT_TONES.index(tone) + amount) % 12]
    end
  end

  def convert_tone(tone, amount)
    sharp?(tone) && amount.negative? ? TONE_MAPPING[tone] : TONE_MAPPING.invert[tone]
  end

  def sharp?(tone)
    !tone.scan(/[A-G]#/).empty?
  end

  def flat?(tone)
    !tone.scan(/[A-G]♭/).empty?
  end

  def change_point_key(amount, range)
    measures = range.size == 1 ? change_single_measure(amount, range) : change_measures(amount, range)
    measures.join('|')
  end

  def change_single_measure(amount, range)
    measures = @melody.split('|')
    tones = change_tones(measures[range[0] - 1], amount)
    measures.delete_at(range[0] - 1)
    measures.insert(range[0] - 1, tones)
  end

  def change_measures(amount, range)
    measures = @melody.split('|')
    add_tones_to_measure(convert_tones(measures[(range[0] - 1)..(range[1] - 1)], amount), measures, range)
    measures.slice!(range[1], range[1] - range[0] + 1)
    measures
  end

  def change_tones(measure, amount)
    measure.scan(/\s|[A-G]#?|[A-G]♭/).map do |tone|
      next tone if /\s/.match? tone

      change_tone(tone, amount)
    end.join
  end

  def convert_tones(measures, amount)
    measures.map do |measure|
      change_tones(measure, amount)
    end
  end

  def add_tones_to_measure(tones, measures, range)
    tones.each.with_index(range[0] - 1) do |measure, index|
      measures.insert(index, measure)
    end
  end
end

フェーズ２　レビューをいただく

ベーコン先輩「じゃあレビューしていくね」

私「よろしくお願いします！」

ベーコン先輩「まずね、 harmonizeメソッドの引数のrangeね、Rangeクラスかと思った。」

ベーコン先輩「でも実際はArrayじゃん？なんやねん！！ってなった。」

私「あーーーーーーーー極端な話、変数名arrayって書いてあるから配列かと思ったらハッシュが入ってる！みたいな……確かに混乱を招いちゃいますね……命名失敗しました。」

ベーコン先輩「次、正規表現ね」

   def change_key(amount)
    @melody.scan(/\||\s|[A-G]#?|[A-G]♭/).map do |tone|
   # 中略
     end
   end

ベーコン先輩「ここ、末尾の記号が#か♭があればマッチでしょ？」

私「はい」

ベーコン先輩「こっちのがわかりやすくない？」

/\||\s|[A-G][#♭]?/

私「あ、」

私「あ〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜〜（クソデカ大声）」

私「まさしくそう……です……そうだと思います……。」

ベーコン先輩「次！sharp?メソッドとflat?メソッドなんだけどね」


  def flat?(tone)
    !tone.scan(/[A-G]♭/).empty?
  end

ベーコン先輩「こっちのがわかりやすくない？」

tone.end_with?('#')

ベーコン先輩「日本語にすると」

私のやつ
- A-Gのいずれかの文字1文字とその次に#な文字列を全て取り出し、配列にして、空では無いか判断する
先輩のやつ
- 最後の文字が#か判断する

ベーコン先輩「どっちがシンプルに見える？」

私「先輩のやつです！！そっか、最後の文字だけ見ればよかった……確かに」

ベーコン先輩「それ以外でいうと、そうだね。今回は既存のものに機能追加をしたじゃん？それなのに変更範囲が多かったかなって。」

ベーコン先輩「なるべく既存のものに変更を加えず、すでにあるものを再利用する形で実装できたらよかったよね」

ベーコン先輩「今回でいうと、私ちゃんが新しく作ったメソッドが多かった。これ、新規で追加するメソッドって２〜３個だよ。」

私「へ」

ベーコン先輩「あとで回答見たら暴れると思う」

私「そんなまさか〜〜〜（このあとめちゃくちゃ暴れた）」

ベーコン先輩「あとは破壊的メソッドが多くて、わかりにくかった。」

ベーコン先輩「破壊的メソッドを実行した後のものを加工していたけど、何が起こっているのかが読み手側としてはわかりにくかった」

ベーコン先輩「それから、破壊的メソッドって実行後に結果が変わるじゃん？」

ベーコン先輩「デバッグするときに事前に止めてその行を実行する。もう一度同じことをすると結果が変わる。一回しか実行できないからデバッグするのに凄い手間がかかる。」

ベーコン先輩「読み手に優しいコードを書くためにも、今の二つは意識して書いた方がいいね。」

私「なるほど……確かにわかりにくい。次から意識してみます！（実際後から見返したらなんのこっちゃって感じで理解するのに苦労した）」

ベーコン先輩「あとはね、私ちゃんは黒鍵のことだけ意識してマッピングを作ってたけど#とか♭が付くのは黒鍵だけじゃないよね」

私「えっ」

私「半音上げたり下げたりって黒鍵だけじゃ、」

無慈悲な生ハムちゃ「ないわね」

私「生ハムちゃ〜〜〜〜〜〜〜〜！？！？！？！？！」

ベーコン先輩「というわけで、黒鍵以外の#や♭の考慮がなかったから、それ系のテストが落ちた。」

私「あああああああああああ（私終了のお知らせ）」

※以下、やばすぎる先輩のソースコード（私は暴れた）↓

class KaraokeMachine
  QUES = '?'.freeze
  SHARP = '#'.freeze
  FLAT = '♭'.freeze
  DEFAULT_TONES = %W[C #{QUES} D #{QUES} E F #{QUES} G #{QUES} A #{QUES} B].freeze

  def initialize(melody)
    @melody = melody
  end

  def transpose(amount)
    return @melody if amount.zero?

    change_key(amount, @melody)
  end

  def harmonize(amounts)
    @melody.split('|').map.with_index do |melody, i|
      next melody if amounts[i].zero?

      change_key(amounts[i], melody)
    end.join('|')
  end

  private

  def change_key(amount, melody)
    melody.scan(/\||\s|[A-G][#{SHARP}#{FLAT}]?/).map do |tone|
      slide_tone(amount, tone)
    end.join
  end

  def slide_tone(amount, tone)
    return tone if tone !~ /([A-G])([#{SHARP}#{FLAT}])?/

    index = slide_tone_index(amount, *Regexp.last_match[1..2])
    result = DEFAULT_TONES[index]

    return result if result != QUES

    amount.positive? ? DEFAULT_TONES[index.pred] + SHARP : DEFAULT_TONES[index.succ] + FLAT
  end

  def slide_tone_index(amount, key, mark)
    index = DEFAULT_TONES.index(key) + amount

    if mark
      index = if mark == SHARP
                index.succ
              else
                index.pred
              end
    end

    index % 12
  end
end

学んだこと

変数名を定義するときは、読み手との認識齟齬を起こさないような名前をつける
正規表現は再度勉強した方が良い（58492901039回思ってるのでそろそろ改めて勉強します）
条件式はなるべくシンプルに書ける方法を考える（日本語にしたときシンプルな条件式はプログラムにしてもシンプル）
読み手に優しいコードを書くことを意識する
- 既存コードの修正の時はなるべく差分が少なくなるように、すでにあるメソッドなどを再利用する
- 破壊的メソッドは基本使わない

まとめ

今回は今までの課題で初めてV2として、機能追加を行いました。自分のコンセプトの元機能追加を行いましたが、追加コードが多く、新規メソッドもたくさん追加することになってしまいました。なるべく修正が少なくなるような仕様、実装を考えないといけないことを痛感しました。そのためにはまず、今自分が使えるアイテムを把握する必要があるので、一旦使えるメソッドを書き出すなどして、いつでも確認できるようにする必要がありそうです。次回から実践します。

条件式をいつも複雑に書いてしまいがちなので、日本語としてシンプルに翻訳できるような条件を考えるよう意識しようと思いました。
また、その瞬間の自分本位なソースコードを書きがちなので、いつ見直しても自分で説明できるようなソースを書くことを意識したいと思います。（今回のソースコードは時間を置いて見直すと？？？？？となってしまった）