hilarysk / rps-project Goto Github PK

(https://github.com/hilarysk/rps-project/blob/master/2015-01-29-rps-player-class.rb#L89)

I noticed you have a lot of explicit returns. That may not be a problem but I know a lot of ruby style guides states that the "Ruby Way" is to return values implicitly (no return statement). Figured I would throw that out there.

Ex:

Avoid return where not required for flow of control.

# bad
def some_method(some_arr)
  return some_arr.size
end

# good
def some_method(some_arr)
  some_arr.size
end

(Linked from: https://github.com/bbatsov/ruby-style-guide#no-explicit-return)

(https://github.com/hilarysk/rps-project/blob/master/2015-01-29-rules-class.rb#L92-L101)

I would suggest thinking about single responsibility rule in some areas. Take the link above as an example.

This method both decides if moves are valid and if they aren't it also judges those moves and increments wins for the players who followed the rules. Seems like it is doing some pretty hefty hauling.

https://github.com/hilarysk/rps-project/blob/master/2015-01-29-game-class.rb#L25-Lundefined

Did you mean for Game to inherit from Player? I can't find where you use that later, and it seems both unnecessary and the sort of thing that could accidentally cause problems that might be hard to diagnose later if you didn't realize you did this.

If you never call Player methods on a game class, I don't think you want this.
Unless I missed where you did?

https://github.com/hilarysk/rps-project/blob/master/2015-01-29-game-class.rb#L44-L49

You don't need either app_name1 or app_name2 as a parameter for this method. You are already passing that info along inside the player objects, it can be accessed by the #name method you defined with attr_accessor in the Player class.

https://github.com/hilarysk/rps-project/blob/master/2015-02-01-psrls-class.rb#L88-L101

In general, you seem to assign @attributes much more frequently than you need to.
If you do not need to recall the information outside of this method, you should not need to save them as attributes.

if you remove
@move1 = move1
@move2 = move
I believe this method will work fine.

Secondly, am I reading this correctly that if I enter "rockl" as my move, or even maybe "Rock" I just LOSE A POINT? That's... harsh. I'm confused, you had a loop that check for valid input earlier when you asked for what kind of game the user wanted to play. Why not use a similar loop when players input their moves? Right now, you're simply penalizing user error. It feels like you're pointing at your user and laughing.

I hesitate to list this one as it comes with knocking out a lot of the character of the returns of these rules but I highly suggest finding the common methods and breaking them out to their own calls. currently psrls has basically the same rules as psr but more.

It would probably take some big reworks but I think it should be doable and cut down a lot of your code as you are repeating yourself quite a bit between the two classes.

For instance: both classes have a method for check_if_moves_are_valid which calls upon their individual @possible_moves array. Aside from the difference that @possible_moves array the methods boil down to the exact same. You should be able to create a module out of these. (Note that currently you are puts different text between the classes but it can be summed up to being the same)

https://github.com/hilarysk/rps-project/blob/master/2015-01-29-game-class.rb#L108-Lundefined

def play_game(game_instance)
@game_instance = game_instance
@game_instance.get_game_choice
@game_instance.instantiate_rules
@rules_instance.print_rules
@game_instance.get_match_num
@game_instance.best_of_loop
@game_instance.deliver_results
@game_instance.print_move_array
end

You are passing a Game object itself as a parameter and setting it as an attribute. I don't think you need @game_instance at all. I believe this can be re-written like this and run exactly the same, and this will also eliminate recursively storing the game class in itself.

def play_game
get_game_choice
instantiate_rules
print_rules
get_match_num
best_of_loop
deliver_results
print_move_array
end

(https://github.com/hilarysk/rps-project/blob/master/2015-01-29-game-class.rb#L131-L139)

Can I ask exactly what a Best of 6 would look like? If you play 6 games and go 50/50 then you end up with a score of 3 to 3. There would need to be a tie breaker in there in this case (making it a best of 7). Should there be another check in there to make sure the number is odd? Roll it around and see if you agree.

https://github.com/hilarysk/rps-project/blob/master/2015-02-01-psrls-class.rb#L116-L195

That is just a BEAST of an if statement.
While I am confident it gets the job done, it causes a few issues:

1. It's hard to read
2. It requires a lot of duplicate code between RPS and RPSLS
3. It can't be fun to debug either.

I used a nested hash to help solve this problem, but I don't think that's the only method (and I stole that idea from Andrew Strovers.)

https://github.com/Ayolland/2015-01-29-rockpaperscissor/blob/master/rules.rb#L57-L80