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

Hannah's Chitter Challenge #232

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

HanzAkor
Copy link

@HanzAkor HanzAkor commented Jul 15, 2022

Hannah Akor

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
Contributor

@siellsiell siellsiell left a comment

Choose a reason for hiding this comment

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

Hey Hannah,

This is a solid solution to the first two user stories, well done. You also showed that you have an understanding of how to alter existing database tables using migrations. I gave you a bit more detailed feedback and pointers below.

If you have any questions, feel free to respond to me here in the comments or send me a DM.

Simo

| ---------- | ------------------------------------------- | --------------------------------- |
| Model | Encapsulate logic with relevant data | Encapsulate peep data in a class |
| View | Display the result to a user | Show the peep data in a list |
| Controller | Get data from the model and put in the view | Render peep data into to the view |
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to see your approach laid out like this, it shows good understanding of the MVC model.

Comment on lines +92 to +109
- list gems in the Gemfile:
source 'https://rubygems.org'

ruby '3.0.2'

gem 'pg'
gem 'sinatra'

group :test do
gem 'capybara'
gem 'rspec'
gem 'simplecov', require: false
gem 'simplecov-console', require: false
end

group :development, :test do
gem 'rubocop', '1.20'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically you'd leave details like that out of the README since those are already in your code. Someone that just wants the code will skip straight to how to install depedencies, tests and how to run the app (what you have described below). Someone interested in more details can take a look at the code inside the files.

Comment on lines +6 to +12
# configure :development do
# register Sinatra::Reloader
# end

# get '/' do
# 'Hello World'
# end
Copy link
Contributor

Choose a reason for hiding this comment

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

It's always a good idea to remove test code and commented out code from your files before you submit for code review (anywhere, not just at Makers). It's all about clean code :)

@@ -0,0 +1 @@
ALTER TABLE peeps ADD COLUMN username VARCHAR(60);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice migration, it's easy to forget to add those!

Comment on lines +26 to +30
if ENV['ENVIRONMENT'] == 'test'
connection = PG.connect(dbname: 'chitter_test')
else
connection = PG.connect(dbname: 'chitter')
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This piece of code appears several times in this file. How might you remove this duplication?

Comment on lines +34 to +38
# The first argument is our SQL query template
# The second argument is the 'params' referred to in exec_params
# $1 refers to the first item in the params array
# $2 refers to the second item in the params array
"INSERT INTO peeps (username, message) VALUES($1, $2) RETURNING id, username, message;", [username, message]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

fill_in('message', with: 'An example of a peep')
click_button('Submit')

expect(page).to have_content 'Username, An example of a peep'
Copy link
Contributor

Choose a reason for hiding this comment

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

A solid feature test!

# visit('/')
# expect(page).to have_content "Hello World"
# end
# end
Copy link
Contributor

Choose a reason for hiding this comment

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

You can feel free to remove any seed files once you don't need them anymore :)

Comment on lines +25 to +29
expect(peeps.length).to eq 3
expect(peeps.first).to be_a Peep
expect(peeps.first.id).to eq peep.id
expect(peeps.first.username).to eq 'User1'
expect(peeps.first.message).to eq 'This is a peep!'
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great that you checked that every field in the peep matches what you expect.
Could you go further?
What if, for example, a bug was introduced in the future that causes Peep.all to return the first peep 3 times? Would your test catch this bug?

Since peeps have three things in it, a thorough test would be to check that each item in the array matches what you expect, not just the first one.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you Simo for your feedback!
This did happen (returning the first peep 3 times) and I didn't know how to deal with it.
I do hope to get the chance to come back and have another go at it 😊.

# message = params['message']
# connection = PG.connect(dbname: 'chitter_test')
# connection.exec("INSERT INTO peeps (message) VALUES('#{message}')")
Peep.create(message: params[:message], username: params[:username])
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a username is a nice idea :)

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