Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Team Quartzy- Annalee, Meighan, Riley, Audrey #16

Open
wants to merge 493 commits into
base: quartzy/master
Choose a base branch
from
Open

Team Quartzy- Annalee, Meighan, Riley, Audrey #16

wants to merge 493 commits into from

Conversation

Dreedle
Copy link

@Dreedle Dreedle commented Dec 19, 2015

App deployed on Heroku:
http://quartzy.herokuapp.com/

Completed primary requirements.
92.6% rspec coverage
Code could be much dryer and neater in some areas, but we feel we accomplished a lot of work.

describe "GET 'cart'" do
it "sets cart status to empty if there is no order_id in session" do
get :cart, {}, {:order_id => nil}
expect(@cart_status).to eq(nil)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this @cart_status is not the same @cart_status in your orders_controller. Specifically this is an instance variable for the RSpec test, so it will be nil forever until it is assigned in the above let block.

@acmei
Copy link

acmei commented Jan 14, 2016

Congrats on completing the Betsy project! The app works wonderfully and you all obviously have a good grasp on complicated logic in rails. Here are some things I noticed overall that could use some improvement:

Naming: Try not to use letters as variables. It's always best to be descriptive when assigning names to data or functions so that your logic is clearer.
Unused Code: There were lots of comments or even functions left in that seemed to go unused. Before making a PR, look over your files to remove these lines or files.
Code Style: Indentation and spacing was kinda weird. Make sure functions are separated by a blank line, logic of the function is contained below the function name and indented (and even separated out into function paragraphs if the logic is complicated), html children are nested beneath parents, etc. I'd recommend looking up a ruby style guide and reading through. ALSO double check to make sure your text editor is set to spaces rather than tabs.
Bangs (:exclamation:): There were lotsa bangs, and sometimes they were completely unnecessary. Always go for the simpler solution!!!!!!!!!!!!!!!!!!!!! See what I did there?
• Make sure your function or logic is contained in the correct file. If the function is helping to render something in the view, make it a helper method. If it's logic is affecting the object itself, put it in the model. This can be really confusing. Just keep at it and it'll make more sense if it doesn't already.

CODE ON! ⭐ ✌️

expect(subject).to render_template :index
end
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to create all these users, categories, and products to test if the index page renders. In any case, what would happen if, say, there were no products to display?

EDIT: Looking at your welcome controller, I see why you needed to create all these. Still might be good to have logic that allows for edge cases like no products to display.

<h2 class="merchant-name">
<%= link_to "#{cat.name}", category_path(cat), class: 'category-box' %>
</h2>
<% cat.top(3).each do |p| %>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, it's better to use full variable names instead of letters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants