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

Betsy Complete - Team Fuzzy #12

Open
wants to merge 422 commits into
base: fuzzy/master
Choose a base branch
from

Conversation

noglows
Copy link

@noglows noglows commented Dec 18, 2015

Project complete. Heroku link provided in the ReadMe file.

A couple little bugs but mostly functional.

Lauren Granger and others added 30 commits December 11, 2015 14:46
end

def update
my_order
Copy link

Choose a reason for hiding this comment

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

You could create a before action to call my_order for checkout and update.

@a-lmx
Copy link

a-lmx commented Jan 12, 2016

From what I looked over, your code looks good. You're using advanced techniques and shortcut methods, and it's clear you understand complicated logic. My challenge for you for your next projects is to focus on clarity over complicatedness. Try to think about a dev reading your code who didn't have anything to do with writing it, and work to write it clearly enough that that future dev could understand it. This means leaving comments when logic is too broad for its purpose to be apparent, being careful about your formatting (white space, indenting), questioning whether you need a ! or if you could use a positive if (eg. in the if !___.nil? methods), and removing unused code. If you have extra time, instead of doing advanced features like Javascript or cookies, you could focus on making the basics really tight and clear.

👍 Keep up the good work!

session[:message] = "You have created a new category: #{@category.name}"
redirect_to user_path(user_id)
else
if @category.errors.messages[:name].include? "can't be blank"

Choose a reason for hiding this comment

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

Neat conditional error messages. Glad that you dug into the errors for invalid records.

@blip-lorist
Copy link

Test coverage is 97.31%. Nice!

@blip-lorist
Copy link

I noticed several flash.now[:error] conditional checks throughout your views. You can dry up your code by including it in your application layout.

@carjug
Copy link

carjug commented Jan 13, 2016

Nice Test coverage!
Your code is laid out well and most of your logic is separated where it should be.
You could improve your project by extracting the responsibility of some model methods into helper methods for the views. Also, be sure to remove commented out code and to make comments in your code for clarity about what certain methods are doing/where they're being called from.

Nice work! Congrats on being done with bEtsy!

<div class="row">
<div class="col-sm-9">
<div class="btn-group" data-toggle="buttons">
<% 1.upto(5) do |i| %>
Copy link

Choose a reason for hiding this comment

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

Hadn't seen upto used -- neat!

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.

7 participants