Skip to content

Jon Clarke Chitter pull request#208

Open
JonClarke84 wants to merge 9 commits into
makersacademy:mainfrom
JonClarke84:main
Open

Jon Clarke Chitter pull request#208
JonClarke84 wants to merge 9 commits into
makersacademy:mainfrom
JonClarke84:main

Conversation

@JonClarke84
Copy link
Copy Markdown

@JonClarke84 JonClarke84 commented Apr 1, 2022

Your name

Jon Clarke

User stories

Please list which user stories you've implemented (delete the ones that don't apply).

  • User story 1: "I want to see all the messages (peeps) in a browser"
  • User story 2: "I want to post a message (peep) to chitter"
  • User story 3: "I want to see the date the message was posted"
  • User story 4: "I want to see a list of peeps in reverse chronological order"
  • User story 5: "I want to filter on a specific keyword"

README checklist

Does your README contains instructions for

  • how to install,
  • how to run,
  • and how to test your code?

Here is a pill that can help you write a great README!

@@ -0,0 +1,15 @@
require 'pg'

class DatabaseConnection
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great idea to split the database connection into a separate class - I hadn't thought to do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed


class << self
attr_reader :connection
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.

How does this class << self function? I haven't seen an attr_reader put inside a method like that before

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 i a bit of metaprogramming.
A mix of: https://yehudakatz.com/2009/11/15/metaprogramming-in-ruby-its-all-about-the-self/
And the fact that all that
attr_reader :connection really does is:

def connection
  @connection
 end

So putting it all together, all these lines do is say:
Any time the connection class is used, it has a method called connection that will return the @connection variable value.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Another way to write this could have been:

def self.connection
    @connection
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 allows users of the class to use DatabaseConnection.connection

.chitter-button:active {
box-shadow: 0 0 0 0 darkred;
transform: scale(96%);
}
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 impressed you put together a CSS style sheet! I'd love to see your webpage when we are on zoom. I wanted to give this a go but didn't end up having the time. It's really useful to see how a .css is laid out for my own understanding.

Comment thread views/search.erb

<ul>
<% @peeps.reverse.each do |peep| %>
<li><%= peep.peep %> Posted on <%= peep.date %>.</li>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How does this peep.peep work? What is the .peep method?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@estoltzy this is the message of the peep.

@JonClarke84 - in terms of naming, the word "message" or "content" might be more meaningful.

Copy link
Copy Markdown

@alicelieutier alicelieutier left a comment

Choose a reason for hiding this comment

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

Great project overall, I like that you added styling.
I've added some comments, let me know if you have any questions.

Comment thread app.rb
Comment on lines 13 to 15
get '/test' do
'Test page'
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.

Detail - remove unused code

Comment thread app.rb
Comment on lines +17 to +25
get '/' do
@peeps = Peeps.all
erb :index
end

post '/add' do
Peeps.create(peep: params[: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.

If you wanted to make this more "standard", you could follow the restful routes naming conventions here: https://medium.com/@shubhangirajagrawal/the-7-restful-routes-a8e84201f206

@@ -0,0 +1,15 @@
require 'pg'

class DatabaseConnection
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed


class << self
attr_reader :connection
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 i a bit of metaprogramming.
A mix of: https://yehudakatz.com/2009/11/15/metaprogramming-in-ruby-its-all-about-the-self/
And the fact that all that
attr_reader :connection really does is:

def connection
  @connection
 end

So putting it all together, all these lines do is say:
Any time the connection class is used, it has a method called connection that will return the @connection variable value.


class << self
attr_reader :connection
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.

Another way to write this could have been:

def self.connection
    @connection
end

Comment thread lib/peeps.rb

def self.create(peep:)
result = DatabaseConnection.query(
"INSERT INTO peeps (message) VALUES ('#{peep}') RETURNING id, message, date;"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check this step walkthrough from the challenge to protect this agains SQL injections - https://github.com/makersacademy/course/blob/main/apprenticeships_bookmark_manager/walkthroughs/12.md

Comment thread lib/peeps.rb
end

def self.all
result = DatabaseConnection.query("SELECT * FROM peeps")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

detail - add ; at end of query.

add_row_to_test_database
visit('/')
expect(page).to have_content "This is a peep! Posted on 2022-04-01."
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.

You can add a test for reverse chronological order. Check how you can use capybara matchers to match a regular expression.

Comment thread lib/peeps.rb
@@ -0,0 +1,43 @@
require_relative 'database_connection'

class Peeps
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

By convention, class names should be singular, table names plural.

Comment thread views/search.erb

<ul>
<% @peeps.reverse.each do |peep| %>
<li><%= peep.peep %> Posted on <%= peep.date %>.</li>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@estoltzy this is the message of the peep.

@JonClarke84 - in terms of naming, the word "message" or "content" might be more meaningful.

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.

3 participants