Skip to content

Makers Chitter Challenge March 23 Cohort - #2171

Open
jonpillay wants to merge 3 commits into
makersacademy:mainfrom
jonpillay:main
Open

Makers Chitter Challenge March 23 Cohort#2171
jonpillay wants to merge 3 commits into
makersacademy:mainfrom
jonpillay:main

Conversation

@jonpillay

Copy link
Copy Markdown

All functionality working as pre spec, except for email. Needs CSS.

@penguat penguat left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Jon - i think there are a few places where you could have reviewed this yourself to see unnecessary code! Your structure looks good, and your code is clear, although you don't always break it up with newlines. You might consider when you need to make new methods to help out too.

Comment thread bcrypt.rb

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure how this contributes to Chitter - could we delete it?

Comment on lines +20 to +25
get '/' do
peeps_repo = PeepRepository.new
@user_repo = UserRepository.new
@peeps = peeps_repo.all
erb(:"app/index")
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I love this, super clean and clear!

Comment on lines +27 to +39
post '/new' do
content = params[:content]
author = session[:user_id]
peep = Peep.new
peep.content = content
peep.author = author
peep_repository = PeepRepository.new
if peep.content.match(/@\w+/)
puts "we are here"
end
peep_repository.create(peep)
redirect '/'
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This feels like it's doing a lot - could it be broken up to make it more digestible?

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.

2 participants