8 Replies - 879 Views - Last Post: 11 July 2013 - 07:56 PM Rate Topic: -----

#1 steve.v  Icon User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 30
  • Joined: 31-October 12

Small Program FeedBack

Posted 01 July 2013 - 11:34 AM

Hello Community,

I have just created my first ruby program and I would like some feedback on it. I am very excited about learning this new language and have come to learn many things about it these last couple of days. I am coming from a basic/intermediate level C++ and C# background and I am just expanding my knowledge to learning other languages. I am still fairly new at programming and I am doing as much as I can reading the wonderful resources by community experts here. Please let me know how I can improve this small Rock, Paper, Scissor Program further to achieve a more readable and possible standard approach using the ruby way. I would like to further minimize the amount of lines in this program and use best practices that I haven't yet learned. Please hit me with all of your knowledge and feel free to ridicule me about the code if required. Thank You.


class Game
  
  def initialize
    
    # Calling the introduction method
    introduction
    
    if playGame
      
      # Calling the playerChoices method
      playerChoices
      
      print "\nChoose one Player 1: "
      @player1_Choice = gets.chomp
      playerChoices
      print "\nChoose one Player 2: "
      @player2_Choice = gets.chomp
      
      # Calculating the game results method
      gameResult
    else
      print "very well, next time then."
    end
  end
  
  def introduction
    print "Welcome to a game of Rock, Paper and Scissors!\n"
    
  end
  
  def playerChoices
    print "\nPlease choose your destiny!\n"
    print "1.Rock\n2.Paper\n3.Scissors\n"
  end
  
  def playGame
    print "Would you like to play a game? Y/N : "
    @play = gets.chomp.upcase
    if @play == "Y"
      return true
    else
      return false
    end
    
  end
  
  def gameResult
    print "\nOkay the winner of the game is: \n"
    player1 = @player1_Choice
    player2 = @player2_Choice
    if player1 > player2
      print "Player 1 Wins!!!"
    elsif player1 < player2
      print "Player 2 Wins!!!"
    else
      print "It is a TIE!!!"
    end
  end
  
end




Is This A Good Question/Topic? 0
  • +

Replies To: Small Program FeedBack

#2 jon.kiparsky  Icon User is online

  • Pancakes!
  • member icon


Reputation: 7650
  • View blog
  • Posts: 12,905
  • Joined: 19-March 11

Re: Small Program FeedBack

Posted 01 July 2013 - 12:22 PM

I see one problem with your game play - scissors should not beat rock! You might want to think about that one.

Stylistically, it looks okay - the code is readable and the variable names are okay.

Architecturally, you might want to return a value from gameResult instead of printing it from the method. That's generally going to provide better flexibility, since it separates game logic from presentation. You could also look at the different methods you're using. I see some room to refactor there, but nothing major.
Was This Post Helpful? 2
  • +
  • -

#3 steve.v  Icon User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 30
  • Joined: 31-October 12

Re: Small Program FeedBack

Posted 01 July 2013 - 12:29 PM

View Postjon.kiparsky, on 01 July 2013 - 12:22 PM, said:

I see one problem with your game play - scissors should not beat rock! You might want to think about that one.

Stylistically, it looks okay - the code is readable and the variable names are okay.

Architecturally, you might want to return a value from gameResult instead of printing it from the method. That's generally going to provide better flexibility, since it separates game logic from presentation. You could also look at the different methods you're using. I see some room to refactor there, but nothing major.


What in the world!? I don't know what I was thinking. I'll have to rethink the algorithm. Thank you for pointing it out. Separate logic from presentation, got it!
Was This Post Helpful? 0
  • +
  • -

#4 Lemur  Icon User is offline

  • Pragmatism over Dogma
  • member icon


Reputation: 1359
  • View blog
  • Posts: 3,425
  • Joined: 28-November 09

Re: Small Program FeedBack

Posted 01 July 2013 - 03:24 PM

# Calling the introduction method
puts "Welcome to a game of Rock, Paper and Scissors!\n"
  
if playGame
  # Calling the playerChoices method
  puts "\nPlease choose your destiny!\n"
  puts "1.Rock\n2.Paper\n3.Scissors\n"
  
  puts "\nChoose one Player 1: "
  p1_choice = gets.chomp

  puts "\nChoose one Player 2: "
  p2_choice = gets.chomp
  
  # Calculating the game results method
  gameResult(p1_choice,p2_choice)
else
  puts "very well, next time then."
end

def playGame
  puts "Would you like to play a game? Y/N : "
  gets.chomp.upcase == 'Y' ? true : false   
end

def gameResult(player1,player2)
  puts "\nOkay the winner of the game is: \n"
  
  # Reimplement
  # if player1 > player2
    # puts "Player 1 Wins!!!"
  # elsif player1 < player2
    # puts "Player 2 Wins!!!"
  # else
    # puts "It is a TIE!!!"
  # end 
end


A little bit nicer. The class is really unnecessary for this. Remember that they're not exactly necessary 100% of the time. Ruby also has implied returns, the last value set is what it assumes.
Was This Post Helpful? 1
  • +
  • -

#5 xclite  Icon User is online

  • LIKE A BOSS
  • member icon


Reputation: 902
  • View blog
  • Posts: 3,163
  • Joined: 12-May 09

Re: Small Program FeedBack

Posted 02 July 2013 - 08:25 AM

View PostLemur, on 01 July 2013 - 06:24 PM, said:

def playGame
  puts "Would you like to play a game? Y/N : "
  gets.chomp.upcase == 'Y' ? true : false   
end



A little bit nicer. The class is really unnecessary for this. Remember that they're not exactly necessary 100% of the time. Ruby also has implied returns, the last value set is what it assumes.

Always love finding these.
gets.chomp.upcase == 'Y' ? true : false is equivalent to gets.chomp.upcase == 'Y'
Was This Post Helpful? 3
  • +
  • -

#6 Lemur  Icon User is offline

  • Pragmatism over Dogma
  • member icon


Reputation: 1359
  • View blog
  • Posts: 3,425
  • Joined: 28-November 09

Re: Small Program FeedBack

Posted 02 July 2013 - 12:00 PM

View Postxclite, on 02 July 2013 - 10:25 AM, said:

View PostLemur, on 01 July 2013 - 06:24 PM, said:

def playGame
  puts "Would you like to play a game? Y/N : "
  gets.chomp.upcase == 'Y' ? true : false   
end



A little bit nicer. The class is really unnecessary for this. Remember that they're not exactly necessary 100% of the time. Ruby also has implied returns, the last value set is what it assumes.

Always love finding these.
gets.chomp.upcase == 'Y' ? true : false is equivalent to gets.chomp.upcase == 'Y'


Derp. That is all.
Was This Post Helpful? 1
  • +
  • -

#7 NotarySojac  Icon User is offline

  • D.I.C Regular
  • member icon

Reputation: 53
  • View blog
  • Posts: 428
  • Joined: 30-September 10

Re: Small Program FeedBack

Posted 08 July 2013 - 04:11 PM

View Poststeve.v, on 01 July 2013 - 11:34 AM, said:

Hello Community,

I have just created my first ruby program and I would like some feedback on it. I am very excited about learning this new language and have come to learn many things about it these last couple of days. I am coming from a basic/intermediate level C++ and C# background and I am just expanding my knowledge to learning other languages.


First off, the code looks pretty good for a new user. I'd like to mention that one of the most amazing things about ruby is gems (package, application, and dependency handling infrastructure), have you heard about gems yet? It's easy to make this into a gem, nah! trivial if you use commands like bundle gem rps. Also, another powerful tool that rubyists use is git. This code would look really beautiful on github... in fact (*alt tabs for 25 seconds into terminal) now it's on git, check it out.

https://github.com/thenotary/rps

Spoiler


From there you'd probably like to see about turning it into a gem so your ruby program is quick and easy to deploy on any PC in the world running ruby (just type `gem install your_gem_name` and gem pulls it down from the Internet and installs it). It's also good to figure out how programs are split into multiple files to make them more clear (ie, the presentation code should be in a seperate file from the logic code). Feel free to ask any follow up question (or just leave remarks on your reactions as you find time to study what's been discussed here, this forum is kind of a slow one that really doesn't get too many "help vampires" as they're sometimes called).

After looking at your code in github, I felt like having that method named "gameResult" was misleading. It isn't returning a result, it's a method that "calculate_winner". (btw, we use underscores to separate words in variables and method names because it's easier to look at our code casually). And as others have mentioned, you've given that method too many responsibilties, not only is it responsible for calculating something, but it's also responsible for presentation. That kind of refactor will really come in handy if you go the extra mile and start unit testing this code too, which I highly recommend you do before you move on to the next language to explore.

http://net.tutsplus....n-with-bundler/
http://stackoverflow...-to-write-a-gem

This post has been edited by NotarySojac: 08 July 2013 - 04:20 PM

Was This Post Helpful? 1
  • +
  • -

#8 NotarySojac  Icon User is offline

  • D.I.C Regular
  • member icon

Reputation: 53
  • View blog
  • Posts: 428
  • Joined: 30-September 10

Re: Small Program FeedBack

Posted 08 July 2013 - 09:32 PM

I couldn't resist creating a branch and refactoring a bit which you can see in a new branch of the code I've created. Specifically note how I'm using a function called 'puts' which automatically puts a \n at the end of the string. It's pretty handy. I'm often guilty of placing lone puts statements just to create new lines, rather than using the \n embedded in my strings, though that creates more lines.

Also notice, I used symbols in a manor that I found humorous and 'lucky', though realistically that approach isn't very flexible.

https://github.com/t.../tree/thenotary

This post has been edited by NotarySojac: 08 July 2013 - 09:35 PM

Was This Post Helpful? 1
  • +
  • -

#9 steve.v  Icon User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 30
  • Joined: 31-October 12

Re: Small Program FeedBack

Posted 11 July 2013 - 07:56 PM

View PostNotarySojac, on 08 July 2013 - 09:32 PM, said:

I couldn't resist creating a branch and refactoring a bit which you can see in a new branch of the code I've created. Specifically note how I'm using a function called 'puts' which automatically puts a \n at the end of the string. It's pretty handy. I'm often guilty of placing lone puts statements just to create new lines, rather than using the \n embedded in my strings, though that creates more lines.

Also notice, I used symbols in a manor that I found humorous and 'lucky', though realistically that approach isn't very flexible.

https://github.com/t.../tree/thenotary


hi NotarySojac,

I haven't really explored what gems are but I will soon after a few programs. It really helped me tremendously looking at your code next to mine. I can see a lot of great ideas and structure. Thank you for your enormous support and feedback.
Was This Post Helpful? 1
  • +
  • -

Page 1 of 1