✉️

【リファクタリング】コードレビューしてもらって学んだこと

2025/01/21に公開

✒️記事を書いた経緯

エンジニアの友人に、コードレビューをしていただきました!
以下、アドバイスしていただいたことをまとめます。


✅リファクタリング

プログラムの動作を変えず、内部構造を修正して、保守や可読性をよくする。


✅コメントアウトの使い方

今までは「何を書いてるか」を書いていたが、正しくは「なぜそうしないか」を書く。
「通常XXになりますが、今回は○○のため、この書き方をしています」…という感じ。

変数名がどれがなんだかわからなくなってしまうので、「何を書いたか」を書いてしまった。
現在の会社では、optionなど汎用性の高いものを使用しているため、その影響だと思う。
通常は、パッと見て何を書いたかわかるような名前にするので、「何を書いたか」は不要。

以下は抽象的な変数を使っている理由。(推測ですが…)

  • プロジェクトを「1から開発している」のではなく、過去のコードやライブラリ、モジュールを再利用している
  • Perlという古い言語を使用しているため、変数名の命名に対する意識が低い?
  • スピード重視の開発のため、一つずつ変数名を考えない。確かに数が多く、一個の案件にあまり時間をかけていない。

✅コードの改行すべきところ、しないところ

間違った書き方

class Character

    def hogehoge
        puts "こんにちは" 
    end

end
  • endendの間空けない!
  • classdefの間空けない!
  • 一番最後のendの下は一行空ける。それ以上は空けない。

正しい書き方

Class Character
    def hogehoge
        puts "こんにちは" 
    end
end


✅単一責任の原則

SOLID原則 参考文献・オブジェクトの広場様
Sが「Single Responsibility Principle」
1つのクラスは1つだけ責任を持つことができる。

最初のコード

  • クラスが防御の情報を保持している⇒防御はその時々によって変わるので、保持する必要がない。
  • キャラクターの攻撃・敵の攻撃と、コードが長いので、まとめたい。
class Character
  def calculate_damage(attack_power)
    defense_value = defend
    damage = [attack_power - defense_value, 0].max
    take_damage(damage)
    damage
end

loop do
  # キャラクターの攻撃
  attack_power = character.attack
  enemy.calculate_damage(attack_power)

  if judge(character)
    break
  end

  # 敵の攻撃
  attack_power = enemy.attack
  character.calculate_damage(attack_power)

  if judge(enemy)
    break
  end
end


修正したコード

  • キャラクタークラスは、ダメージ計算メソッドだけ保持。
    (ダメージ計算による受けた攻撃は、キャラクターが保持する必要があるため)
  • execute_turn(ターン実行)メソッドをmain.rbに書いて、引数をattackerとdefenderとする。これでキャラクターの攻撃・敵の攻撃と、長かったコードを解消!
  • judgeメソッドでは何をjudgeしてるかわかりづらい⇒judge_defeat(敗北者)に変更。
class Character
  def calculate_damage(attack_value, defense_value)
    [attack_value - defense_value, 0].max
  end
end

def judge_defeat(target)
  if target.hp <= 0
    puts "#{target.name} は倒れた!"
    return true
  end
  false
end

def execute_turn(attacker, defender)
  attack_value = attacker.attack
  defense_value = defender.defend
  damage = attacker.calculate_damage(attack_value, defense_value)
  defender.take_damage(damage)

  judge_defeat(defender)
end

loop do
  break if execute_turn(character, enemy)
  break if execute_turn(enemy, character)
end


出力結果

PS C:\Users\xxx\Desktop\ruby_test> ruby main.rb
hero は攻撃した!
devil は防御した!
devil は 0 のダメージを受けた!残りHP: 100
devil の残りHPは 100 です。
devil は攻撃した!
……省略……
hero は 4 のダメージを受けた!残りHP: 0
hero の残りHPは 0 です。
hero は倒れた!

Discussion