Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Develop janken program #1

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Develop janken program #1

wants to merge 3 commits into from

Conversation

potechi-maru
Copy link
Owner

概要

じゃんけんプログラムを作成

手を加えたファイル

  • main.rb
  • README.md

main.rb Outdated
Comment on lines 17 to 22
if @number_of_times == 3 || @number_of_times == 5
@number_of_times.times do
janken
end
announcement
else # number_of_times == 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここってif~elseにする必要あります?1がきても@number_of_times.times doの処理が行っても問題ないんじゃないかなと思いましたがどうでしょう?

main.rb Outdated
@cpu = rand(3) # 0..2どれかランダムに表示
player = gets.chomp
@player = press[player] # pressのkeyからval取得
if player == "g" || player == "c" || player == "p"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここはもっとシンプルに

if @prayer.nil?
    puts "g, c, pから選んでください"
    battle
else
    puts "CPU…#{jankens[@cpu]}"
    puts "あなた…#{jankens[@player]}"
    result
end

に置き換えられそう。

main.rb Outdated
if @cpu == @player
puts "あいこで…(press g or c or p)"
battle
elsif (@cpu == 1) && (@player == 0) || (@cpu == 2) && (@player == 1) || (@cpu == 0) && (@player == 2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

現状パッとみてわかりづらいですね。勝ち負けのロジックをメソッドに切り出すと可読性があがりそうです。例えばですがこういうメソッドを作ってそっちにロジックを移してみたらどうでしょう?

def win?

end

main.rb Outdated
else
puts "負け!"
@lose += 1
puts @results = "#{@win}勝#{@lose}敗"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

始めはif文の外にputs @results = "#{@win}勝#{@lose}敗"に記載していましたが、あいこになった回数だけ表示されてしまいました。

そのためif文の中に組み込みましたが全く同じ文が重複しており気になります。

リファクタリングのヒントをいただけますと嬉しいです。

例えばですが、あいこの時は早期returnしてしまうのはどうでしょうか?if~elseではなく早期returnです。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def win?
  if  @cpu == @player
    puts "あいこで…(press g or c or p)"
    battle
    return
  end
  if (@cpu == 1) && (@player == 0) || (@cpu == 2) && (@player == 1) || (@cpu == 0) && (@player == 2)
    puts "勝ち!"
    @win += 1
  else
    puts "負け!"
    @lose += 1
  end
end

def result
    win?
    puts @results = "#{@win}勝#{@lose}敗"
end

この場合の早期リターンとはこういうことでしょうか?スマホからなのでインデント微妙かもしれません。
他の項目についてのご指摘もありがとうございます。帰宅後修正いたします。

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

例えばこうとかはどうでしょう?

def result
  if draw?
    puts "あいこで…(press g or c or p)"
    battle
    return
  end

  if win?
    puts "勝ち!"
    @win += 1
  end

  if lose?
    puts "負け!"
    @lose += 1
  end

  puts @results = "#{@win}#{@lose}敗"
end

def draw?

end

def win?
  (@cpu == 1) && (@player == 0) || (@cpu == 2) && (@player == 1) || (@cpu == 0) && (@player == 2)
end

def lose?
  !draw? && win?
end

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほど、メソッドに移すことでラベリングっぽくしたいということですかね。書く量は増えるけど可読性が上がるということですね。この方がresultメソッドがわかりやすいです!

質問が2つあります。

  • draw?メソッドの中身は==で比較でよろしいでしょうか。
  • lose?メソッドの否定演算子!はひとつでwin?メソッドも否定になるのでしょうか。!(draw? && lose?)というイメージでしょうか。

Copy link

@miketa-webprgr miketa-webprgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

すみません、さらっと見ただけですが、一応コメントしましたのでご確認ください!

Comment on lines +10 to +13
else
puts "1, 3, 5から選んでください"
start
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1, 3, 5 以外の入力があった場合のパターンも考慮できていて、すばらCですね。レモンはビタミンCですね。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ビタミンCいただきました!ありがとうございます!

@cpu = rand(3) # 0..2どれかランダムに表示
player = gets.chomp
@player = press[player] # pressのkeyからval取得
if @player.nil?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

本当は、g, c, p 以外のものを入力した場合に「g, c, pから選んでください」と再度表示させてあげるべきなので、愚直に書こうとすると、こんな感じですかね

if @player == 'g'  || @player == 'c' || @player == 'p'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あとは、こんな感じでもいけますね。
もう少し、この箇所だけでなく他もこねくり回すと、スマートな形で書けるかと思います。

if press.keys.include?(@player)

いちおう、理解しやすいように補足しておきます!

press.keys #=> ["g", "c", "p"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あ、けど、@player = press[player]で想定したvalues以外の場合だとnilが返るようにしているんですね。失礼しました 🙇

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

実はこの部分ははじめ、おっしゃる通り文字列との比較をしていたんです。ですが、スマートじゃないのかなーと思いまして数値に直しました。

if press.keys.include?(@player)

確かにこの方法もありですね!自分ではincludeメソッドが出てこなかったです。勉強になります!



def result
if (@cpu == 1) && (@player == 0) || (@cpu == 2) && (@player == 1) || (@cpu == 0) && (@player == 2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この箇所は、おそらく間違えではないんでしょうけど、ロジックをミスしやすそうなので、個人的には可読性が高い形に置き換えたいです。おそらく、@cpu = rand(3)の箇所をそのまま生かしたいということだと思うのですが、できれば、以下のような形にしたいですよね。

(@cpu == 'g') && (@player == 'p') || (@cpu == 'c') && (@player == 'g') || (@cpu == 'p') && (@player == 'c')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

その場合、@cpuを数字ではなく、g, c, p にこんな感じで置き換えられるかと思います。

irb(main):014:0> press.invert
=> {0=>"g", 1=>"c", 2=>"p"}
irb(main):015:0> press.invert[rand(3)]
=> "p"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちなみに、こういった書き方については、rubyのドキュメントを読んでいくと、色々と書いてあったりします!

https://docs.ruby-lang.org/ja/latest/library/index.html

rubyはまつもとゆきひろさんが開発したということもあって、日本語でちゃんとしたドキュメントがあるので、気になった際はどんどん確認してみてください!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ロジックをミスしやすそうなので

まさにその通りです!ミスしていてなぜか負けるな?と思ったら1部分だけ数字が逆になってました。
そしてこれも数字の方がいいのかな〜という理由で変更した部分です;

press.invert
=> {0=>"g", 1=>"c", 2=>"p"}

キーを値に、値をキーにするメソッドなんですね。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 press = {"g" => 0, "c" => 1, "p" => 2}
  jankens = ["グー", "チョキ", "パー"]
  @cpu = press.invert[rand(3)]
  @player = gets.chomp

このようにplayerを@playerに変更して、元あった@playerを消せば良いのかなと思ったのですが、そうするとgets.chompがうまく動きませんよね。invertメソッドにする場合は@player側もちょっと仕組みを変えないとですね。

def battle
press = {"g" => 0, "c" => 1, "p" => 2}
jankens = ["グー", "チョキ", "パー"]
@cpu = rand(3) # 0..2どれかランダムに表示

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こういうコメントを書く姿勢はすばらCと思います。シミ・そばかすにはハイチオールCですね。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ハイチオールC探してました。ありがとうございます!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants