Week 4, day 3

Had my code review today and I'm super happy about it. My coach this week is a coder with 30 years experience, a seriously knowledgable and very friendly guy. I think his niceness might temper his feedback slightly, I don't think he was harsh enough on me, but I'll take it!
The Makers Academy course is seriously hardcore and the first three weeks are hellish, for the first time I'm actually feeling like I can stop stressing and have fun.

Hi Sanj,
This is great work. Don't take my comments as indication of anything else. You've done really really well here and mastered a number of very sophisticated aspects of programming such as stubbing random behaviour.
It's partly because you've done so well that I'm focusing in on the areas where there is still room for improvement.
I'm going to give you a sticker and mark you completed because you've got a great product here - and the interface looks great, but I do hope you'll review my comments and think about making some of the changes I suggest, or at least bear them in mind going forward. :checkered_flag:

Detailed feedback and a bit more about last week's challenge after the break.

Given(/^my opponent chooses "([^"]*)"$/) do |arg1|
+  CPU.any_instance.stub(:choice) { arg1 }

great that you are stubbing this, but as discussed, you should prefer the more modern:
allow_any_instance_of(CPU).to receive(:choice).and_return arg1
or the stronger
expect_any_instance_of(CPU).to receive(:choice).and_return arg1

+<div class="left" style="background-color:#FCD57A; background-image: url(https://openclipart.org/image/125px/svg_to_png/213382/rock-paper-scissors.png);"><%= session[:Name] %></div>

great use of background images, although we could replace div with a more semantic tag?

<button type="submit" name="Choice" value="Rock">Rock!</button>

trivial, but I'd prefer lower case for these names --> i.e. 'choice'

<h1>
+  Welcome <%= session[:Name] %>

you should avoid accessing the session directly in your view in order to make the view as generic as possible

<link href='http://fonts.googleapis.com/css?family=Bangers' rel='stylesheet' type='text/css'>
+<link rel="stylesheet" href="<%= url('/styles.css') %>">
+
+<h1>
+  Welcome <%= session[:Name] %>
+</h1>
+
+<h2>
+  You are playing RPSLS against the computer
+</h2>
+
+<div class="left" style="background-color:#FCD57A; background-image: url(https://openclipart.org/image/125px/svg_to_png/213382/rock-paper-scissors.png);"><%= session[:Name] %></div>

you should avoid accessing the session directly in your view in order to make the view as generic as possible

it 'can choose Rock, Paper or Scissors' do
+    choice_array = []
+    100.times { choice_array << cpu.choice }

I think this is a little dangerous - you're relying on a certainly level of randomness from 'sample'.
I think you'd be better off testing the contents of the array that is being drawn on, and that sample was being called on it. Here you are effectively testing the sample method, which you should avoid

get '/' do
+    erb :index
+  end
+
+  get '/game' do
+    redirect '/'
+  end
+
+  post '/game' do
+    if params[:Choice]

this is the logic in your view that I was talking about. You have indeed pulled your game logic into your models/objects, but this switch on the params[:Choice] content is logic that we should avoid.
You should prefer two different post methods here, e.g.
post '/game' do @name = session[:name] erb :game end
post '/result' do @decision = decide.make params[:choice], cpu.choice erb :result to cleanly separate the concerns and reduce the need for logic in the server (controller). I would also be tempted to pull the cpu choice logic into the model, and be able to have the view rely on a single result or decision object for all its data.
Also, I think we might want to think about changing the method names "decide.make" doesn't read very well.

+    <button type="submit" value="PlayRPSLS" onclick="this.form.action='/game_rpsls'">Play RPSLS</button>

this is kind of ugly - I think I would prefer to switch in the controller based on which button was pressed. That might seem to go against the "no logic in the controller" motif that I'm presenting, but I think we could actually pull that logic through into the model objects as well and collapse the rather similar post '/gamerpsls' and post 'game' methods into one.
Also alot of similarities between game.erb and game
rpsls.erb - could be DRYed out, same for result and results_rpsls

Phew, I'll definitely be incorporating these requested changes into my files. Also had a further code review for last week's takeaway challenge, I was given a pass but the coach that reviewed it is quite particular about Object-Oriented design and he does it bloody well.

I'll be making a few final changes to that as well so I can draw a line under it and be happy.

Written on April 8, 2015