From 92bbe591c674e816c587c632b290934da4b13867 Mon Sep 17 00:00:00 2001 From: jleo3 Date: Wed, 27 Nov 2013 18:09:49 -0500 Subject: [PATCH] Comments, suggestions, compliments This app is awesome! I would love to watch it grow and develop. Good luck! --- app/assets/javascripts/map.js | 7 +++++++ app/controllers/kicks_controller.rb | 9 ++++++++- app/controllers/rsvp_controller.rb | 1 + app/controllers/user_controller.rb | 2 ++ app/models/user.rb | 1 + app/views/home/index.html.erb | 3 +++ app/views/kicks/_form.html.erb | 8 +++++++- notes.txt | 6 ++++++ 8 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 notes.txt diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index 16beaec..f2111a3 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -6,6 +6,13 @@ * To change this template use File | Settings | File Templates. */ +/** + * Like I said in notes.txt, this is ALL you! I know you have frontend experience, and it's great to let it shine through here. + * + * One thing that strikes me is that your JS files and most of their functions are a bit long and complex. You might have difficulty changing things. You have lots of comments to remind you what the code does. + * Have you looked at Jasmine (https://github.com/pivotal/jasmine)? Or JS lint (http://www.jslint.com/)? Both are tools that could help you keep your JS complexity down. + */ + load("home#index", function (controller, action) { var config = { clientId: 'BCH20MFU1KWJNGTFVQHYSOQGDA42BZ5KYWGIQJW40HT4PAOT', diff --git a/app/controllers/kicks_controller.rb b/app/controllers/kicks_controller.rb index f6e4fc4..b70e1c1 100644 --- a/app/controllers/kicks_controller.rb +++ b/app/controllers/kicks_controller.rb @@ -1,4 +1,7 @@ class KicksController < ApplicationController + # Currently, this method returns this error when I navigate here: + # "There was an error saving your kick. Sorry!" If you don't want your + # users to access it, you should take it out. def index end @@ -16,6 +19,9 @@ def new end def create + # You can leverage your ActiveRecord associations to do some of this heavy + # lifting. @kick.user = @user should be all you need. Then if you want the + # @kick's username, it's just @kick.user.name @user = current_user kick_params = params.require(:kick).permit(:title, :description, :time, :location, :user_id, :latitude, :longitude, @@ -59,7 +65,8 @@ def update end end - + # In a RESTful world, this would live at kickin-it.com/users/#{user.id}/kicks + # In other words, this logic would live in the #index method def mykicks @user = User.find_by_id(current_user.id) @kicks = @user.kicks diff --git a/app/controllers/rsvp_controller.rb b/app/controllers/rsvp_controller.rb index 2fd630c..7568807 100644 --- a/app/controllers/rsvp_controller.rb +++ b/app/controllers/rsvp_controller.rb @@ -1,2 +1,3 @@ class RsvpController < ApplicationController + # Needed? If not, there's no reason to keep it. Trust in git! end diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 33a7148..262b2e6 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -13,6 +13,8 @@ def show @user = User.find_by_permalink(params[:id]) end + # TODOs are a code smell: http://c2.com/cgi/wiki?CodeSmell. File issues in + # your GitHub repo instead. ##TODO -- refactor from profile controller def update diff --git a/app/models/user.rb b/app/models/user.rb index cadf31f..367aeec 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -19,6 +19,7 @@ class User < ActiveRecord::Base :storage => :s3, :s3_credentials => "#{::Rails.root}/config/s3.yml" + # Nicely done here. Simple, declarative methods. def user_id current_user.id end diff --git a/app/views/home/index.html.erb b/app/views/home/index.html.erb index 6d8c375..908403e 100644 --- a/app/views/home/index.html.erb +++ b/app/views/home/index.html.erb @@ -6,6 +6,9 @@
+ <% =begin %> + This looks like a LOT of work. Great work getting all of this together and functional. + <% =end %> <% @kicks.each do |kick| %>
diff --git a/app/views/kicks/_form.html.erb b/app/views/kicks/_form.html.erb index 3506529..7ee6da0 100644 --- a/app/views/kicks/_form.html.erb +++ b/app/views/kicks/_form.html.erb @@ -1,3 +1,6 @@ +<% =begin %> +I like your use of partials - this and _sidebar.html.erb in particular. Your views leverage partials and stay DRY. Great work! +<% end %>
<%= form_for(@kick) do |f| %> <% if @kick.errors.any? %> @@ -64,6 +67,9 @@
+<% =begin %> +Hey! You know where this belongs! app/assets/javascripts/ +<% =end %> \ No newline at end of file + diff --git a/notes.txt b/notes.txt new file mode 100644 index 0000000..3863733 --- /dev/null +++ b/notes.txt @@ -0,0 +1,6 @@ +- This is one of the best apps in the class! You've done so much outside the scope of the class to get your app up to the level it is now. I love the way Kickin It looks and feels, and it's really easy to navigate. All of that is on you - I know I didn't teach it! + +- Using paperclip and AWS makes the application super-responsive, which I love. You did a great job with this. + +- I gave plenty of comments on your JS and view files. Your views are well done and well-factored and you make great use of the asset pipeline. So when I give you advice on JS, just bear in mind it's because I know you're ready for the next level! +