From 508ae82860f832b080b182b913d94b0a351ba42a Mon Sep 17 00:00:00 2001
From: Samson Ootoovak <sam@enspiral.com>
Date: Mon, 29 Sep 2014 18:04:07 +1300
Subject: [PATCH 1/2] Ruby review.

---
 app/controllers/dashboard_controller.rb | 1 +
 app/controllers/profiles_controller.rb  | 3 +++
 app/helpers/dashboard_helper.rb         | 1 +
 app/models/gift.rb                      | 4 ++++
 app/models/request.rb                   | 2 ++
 app/views/dashboard/index.html.erb      | 3 ++-
 app/views/dashboard/requested.html.erb  | 3 ++-
 config/routes.rb                        | 3 +++
 8 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index e891760..154892d 100644
--- a/app/controllers/dashboard_controller.rb
+++ b/app/controllers/dashboard_controller.rb
@@ -1,4 +1,5 @@
 class DashboardController < ApplicationController
+  # Minor niggle but try to remove trailing whitespace
   
   def index 
   end 
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 9f4fe68..7215a6a 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -1,8 +1,11 @@
 class ProfilesController < ApplicationController
   def show
+    # Don't commit commented out code
     # @user = User.find_by_profile_name(params[:id])
     @user = User.find(params[:id])
     if @user 
+      # By default the association will give you all gifts for this user so you
+      # don't need to call all after.
       @gifts = @user.gifts.all 
       render action: :show 
     else 
diff --git a/app/helpers/dashboard_helper.rb b/app/helpers/dashboard_helper.rb
index a94ddfc..14cd79b 100644
--- a/app/helpers/dashboard_helper.rb
+++ b/app/helpers/dashboard_helper.rb
@@ -1,2 +1,3 @@
+# Don't commit empty files. Any empty helpers should just be deleted.
 module DashboardHelper
 end
diff --git a/app/models/gift.rb b/app/models/gift.rb
index 807f9b5..a8d1d24 100644
--- a/app/models/gift.rb
+++ b/app/models/gift.rb
@@ -1,4 +1,8 @@
 class Gift < ActiveRecord::Base
+  # Similar to whitespace get the whole team on one type of tab
+  # I would suggest 2 spaces for a tab. Seems like a small thing
+  # but different editors show tabs differently and it messes
+  # with indentation making the code harder to read.
 	validates :title, presence: true
   validates :description, presence: true
 
diff --git a/app/models/request.rb b/app/models/request.rb
index 83c28b7..6066be9 100644
--- a/app/models/request.rb
+++ b/app/models/request.rb
@@ -1,4 +1,6 @@
 class Request < ActiveRecord::Base
   belongs_to :user
   belongs_to :gift
+  # Should add validations to make sure the
+  # foreign keys have been added
 end
diff --git a/app/views/dashboard/index.html.erb b/app/views/dashboard/index.html.erb
index 8d0eaff..d55883d 100644
--- a/app/views/dashboard/index.html.erb
+++ b/app/views/dashboard/index.html.erb
@@ -16,6 +16,7 @@
       <% current_user.gifts.each do | gift| %>
         <div class="row">
           <div class="col-xs-2 ">
+            <!-- should be using link_to helper here -->
             <a href="/gifts/<%= gift.id %>">
               <%= gift.title %>
             </a>
@@ -41,4 +42,4 @@
             <% end %>      
 
     </div>
-</div>
\ No newline at end of file
+</div>
diff --git a/app/views/dashboard/requested.html.erb b/app/views/dashboard/requested.html.erb
index 6af5644..16bca14 100644
--- a/app/views/dashboard/requested.html.erb
+++ b/app/views/dashboard/requested.html.erb
@@ -14,6 +14,7 @@
   <ul class="nav nav-tabs" role="tablist">
     <li class="active"><a href="#my-gifts" role="tab" data-toggle="tab">My Gifts</a></li>
     <li><a href="#my-requests" role="tab" data-toggle="tab">My Requests</a></li>
+    <!-- broken link tag, should use link_to. Also could use helper method here to create whole li tags -->
     <li>a href="#profile" role="tab" data-toggle="tab">Profile</a></li>
   </ul>
 
@@ -43,4 +44,4 @@
             <% end %>      
 
     </div>
-</div>
\ No newline at end of file
+</div>
diff --git a/config/routes.rb b/config/routes.rb
index d13f09f..1c65fbf 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -18,6 +18,9 @@
   # Jaz JS learning objectives:
   get 'gifts/:id/jaz', to: 'gifts#jaz'
 
+  # Remove commented out code. I know this is generated by Rails but good to remove
+  # commented out code ASAP as it just adds noise and hangs around long after it
+  # is intended to.
   # The priority is based upon order of creation: first created -> highest priority.
   # See how all your routes lay out with "rake routes".
 

From 28a7750cb4357e7d83d882a5f9437db7e312d2ce Mon Sep 17 00:00:00 2001
From: Samson Ootoovak <sam@enspiral.com>
Date: Mon, 29 Sep 2014 18:19:28 +1300
Subject: [PATCH 2/2] JS review.

---
 app/assets/javascripts/gifts.js.coffee  | 2 ++
 app/assets/javascripts/jazController.js | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/app/assets/javascripts/gifts.js.coffee b/app/assets/javascripts/gifts.js.coffee
index e97c0ec..2064d89 100644
--- a/app/assets/javascripts/gifts.js.coffee
+++ b/app/assets/javascripts/gifts.js.coffee
@@ -3,6 +3,8 @@
 # You can use CoffeeScript in this file: http://coffeescript.org/
 
 
+# Should be consitat with the project and use either all JS
+# or all Coffeescript
 $ ->
   $('.gift').hover (event) ->
     console.log("hover triggered")
diff --git a/app/assets/javascripts/jazController.js b/app/assets/javascripts/jazController.js
index 7692e5e..5e38d2b 100644
--- a/app/assets/javascripts/jazController.js
+++ b/app/assets/javascripts/jazController.js
@@ -18,7 +18,9 @@ Controller.prototype = {
         url: "/gifts.json",
         type: "GET",
         //'for' loop extracts gift data from the JSON & creates gift objects
+        // consider moving this anonymous function into a named function
         success: function(data) {
+          // don't commit debugging statements
           console.log(data);
           self.gifts = [];
           for (var i=0; i<data.length; i++) {
@@ -61,6 +63,8 @@ Controller.prototype = {
     // send the html from the template to the view
     this.view.showGiftDetails(html);
 
+    // don't commit commented out code
+    // use git and Github to refer to old code instead.
     // bind events for the save and delete buttons
     // $(this.view.saveButtonSelector).on("click", this.updateGiftDetails.bind(this));
     // $(this.view.deleteButtonSelector).on("click", this.deleteGift.bind(this));