Skip to content

Chitter - Eleanor CS#218

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

Chitter - Eleanor CS#218
estoltzy wants to merge 9 commits into
makersacademy:mainfrom
estoltzy:main

Conversation

@estoltzy
Copy link
Copy Markdown

@estoltzy estoltzy commented Apr 4, 2022

Your name

Please write your full name here to make it easier to find your pull request.

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!

Copy link
Copy Markdown

@JonClarke84 JonClarke84 left a comment

Choose a reason for hiding this comment

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

This looks great to me :)

Comment thread views/peeps.erb


<% @peeps.each do |peep| %>
<div class="Peeps" style="border: 3px solid aqua; width:fit-content; width:-webkit-fit-content; width:-moz-fit-content;">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The styling would be better placed in a css file I reckon, especially given that this code will repeat on every iteration of the loop.

Comment thread views/peeps.erb
</div>
<% end %>

<form action="/peeps/reverse" method="get">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Love that this is a button (I did mine the lazy way!)

Comment thread lib/peeps.rb

result = connection.exec("SELECT * FROM peeps;")
result.map do |peep|
row_to_peep(peep)
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 line being refactored into its own method is nice

Comment thread lib/peeps.rb
@entry_date = entry_date
end

def self.open_connection
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's possible to refactor this into its own class and have an always on db connection while the app is running. There's a walkthrough on how to do this in Ex.15 on the Bookmark task.

Comment thread lib/peeps.rb
def self.reverse_chronology
connection = open_connection

result = connection.exec("SELECT * FROM peeps ORDER BY entry_date DESC;")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh interesting, I did this with a .reverse method on the array.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Which approach is more computationally efficient? How would you test it?

Comment thread lib/peeps.rb

def self.filter(filter)
connection = open_connection
result = connection.exec("SELECT * FROM peeps WHERE lower(message) LIKE '%#{filter.downcase}%';")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using downcase on everything is better than what I did

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's a nice approach, I agree!

Peeps.create(message: 'Second test peep!', entry_date: "2021-02-15")

visit ('/peeps')
fill_in('filter', with: 'peep')
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given that both test peeps contain the word 'peep' maybe another word could have been used to test the filter function, such as 'Second'? Even better would be 'second' as then you test your downcase code as well.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good points!


click_button "View Newest First"

page.body.index("2021-03-18").should < page.body.index("2021-02-15")
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 have never seen page.body.index before. Nice.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Neither have I :)

Comment thread views/index.erb
@@ -0,0 +1,2 @@
<h1> Welcome to Chitter! </h1>
<img src="https://gl.audubon.org/sites/default/files/styles/hero_mobile/public/aud_apa-2020_barn-swallow_a1-11258-1_nape_photo-xianwei-zeng.jpg?itok=aTU-t6nJ" alt="Chitter Birds"> No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe this picture could link to your /peeps page?

Comment thread app.rb
require 'sinatra/base'
require './lib/peeps'

class Chitter < Sinatra::Base
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 could probably delete this test route.

Comment thread app.rb
'Test page'
end

get '/' do
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

these are generally good RESTful routes – nice work!

You could consider ways of combining the routes for all the requests that show multiple Peeps but, then again, you could also make the argument that such an approach would break SRP.

@EdwardAndress
Copy link
Copy Markdown

This is a really nice solution that clearly hits all the learning objectives. Great work 👍

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