Common issues

Not testing all of the game logic in unit tests

As there is a discrete number of possible outcomes, your tests should test them all. This may seem like overkill, but how else will you know that your game logic is correct in all circumstances?

Testing game logic in feature tests

Your feature test should not need to test all of the rock/paper/scissors(/lizard/spock) possibilities - this is the responsibility of your unit tests.

Not testing all game outcomes in feature tests

Although you do not need to test all possible combinations, your feature tests should test every possible outcome - i.e.:

• a win
• a loss
• and a draw. to ensure the user interface logic is correct.

Including presentation strings in business logic layer

Your Game class (or similar) should not return presentation strings like "Congratulations - you won!". This is a presentation concern and should be handled in another layer of code (separation of concerns). Instead, return representative codes, such as :win and :draw from the Game class which can be translated by the presentation layer.

This approach makes it possible to change the presentation layer (e.g. to add support for a different language) without changing the lower-level code (open/closed principle).

Use of if/elsif conditionals for business logic

Long if and elsif trees are very difficult to read and nested if statements require too much working memory for a reader to quickly scan.

There are a number of approaches to the game logic of Rock Paper Scissors, e.g.:

• Use a hash to map the rules:

rules = { rock: scissors,
          paper: :rock,
          scissors: :paper }

or for RPSLS:

rules = { rock: [scissors, lizard],
          paper: [:rock, :spock],
          scissors: [:paper, :lizard],
          lizard: [:paper, :spock],
          spock: [:rock, :scissors] }

• Use individual classes for each weapon (i.e. Rock, 'Paperetc.) with abeats?` or similar method that takes another weapon as a parameter.

Not encapsulating the 'computer' in a separate class

By creating a Computer class, you can take advantage of duck-typing in the game class. The game does not need to know if it's comparing two players or one player vs a computer or even two computers!

Using global variables

You may use one and only one global variable or class variable to store the game. All other objects should be referenced within the game if necessary.

Not storing the weapons in a constant

If you have something like this:

def weapons
  ['Rock', 'Paper', 'Scissors']
end

Then 4 new objects will be created every time you call weapons. Use a constant with symbols instead:

WEAPONS = [:rock, :paper, :scissors]

Inconsistent file naming

Ruby class files should be named with the snake_case version of the class name. Class names should be PascalCase. Hence:

Good

• class Rps -> rps.rb
• class RpsWeb -> rps_web.rb

Bad

• class RPS_web -> rps_web.rb
• class RpsWeb -> rps.rb

Note, naming conventions tend to prefer acronyms to be 'wordified' i.e. RPS becomes Rps or rps as appropriate.

Defining weapons in more than one place

Don't Repeat Yourself (DRY)! The list of available weapons should be defined in only one place. It can be passed around or referenced or injected, but not duplicated!

POSTing to /result

Sending a POST to /result implies that you are setting the result rather than submitting a go. /play would be better.

Calling business logic from the view

It is the controller's responsibility to pass the player's weapon to the game and get the result. Use instance variables or helper methods to represent or convert this result for rendering in the view.