User stories 1 and 2 completed (temporary solution to user story 4) by jormark · Pull Request #237 · makersacademy/chitter-challenge-apprenticeships

jormark · 2022-07-18T09:16:43Z

Your name

Jordan Markham

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 4: "I want to see a list of peeps in reverse chronological order"

README checklist

Does your README contains instructions for

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

jormark

Overall good test coverage, program works as expected and meets the checked requirements. However, formatting the output for better coherency is required, along with a README with instructions on how to operate the program.

jormark · 2022-07-18T09:18:07Z

+
+class Peep
+  def self.all
+    if ENV['ENVIRONMENT'] == 'test'


There could be a method for this, in order to avoid repeating code!

Good observation! If you hadn't commented on it yourself, I would have 😁

jormark · 2022-07-18T09:19:07Z

+    expect(page).to have_content "Yoyoyo!"
+    expect(page).to have_content "http://www.google.com"
+  end
+end


Always include a new line at the end of a code file!

jormark · 2022-07-18T09:20:04Z

@@ -0,0 +1,10 @@
+feature 'Creating a new peep' do


Test coverage is good, but the types of input (try invalids too?) aren't being tested. Only one type of input is being tested here...

That's a good point. To make your application more robust, you'd want to make sure you caught invalid inputs and test that too. In the context of this challenge, it's a good enough feature test though :)

jormark · 2022-07-18T09:22:00Z

+    <button value="Submit" type="submit">Peep!</button>
+  </form>
+
+  <p><%= @all_peeps.join('/n') %></p>


This wouldn't work - ('/n') would just show up as a string. Maybe "/n" (full quotation marks) would work?

What's the HTML equivalent of a line break? That's the thing to research.

siellsiell

Hey Jordan,

Your code is very clean and readable and you've already made some great observations of your own about how you could improve it. I added a few more comments below.

I was wondering what led you to use the temporary solution to user story 4 -- were just not very keen to implement user story 3 or did you get blocked somewhere?

If you have any questions, just answer on here in the comments or DM me.

siellsiell · 2022-07-20T16:18:33Z

  get '/test' do
    'Test page'
  end


It's always a good idea to remove test code from code you submit for code review (anywhere, not just at Makers). Just keeps things clean and easy to follow.

siellsiell · 2022-07-20T16:25:21Z

+
+class Peep
+  def self.all
+    if ENV['ENVIRONMENT'] == 'test'


Good observation! If you hadn't commented on it yourself, I would have 😁

siellsiell · 2022-07-20T16:27:21Z

@@ -0,0 +1,10 @@
+feature 'Creating a new peep' do


That's a good point. To make your application more robust, you'd want to make sure you caught invalid inputs and test that too. In the context of this challenge, it's a good enough feature test though :)

siellsiell · 2022-07-20T16:28:30Z

+    expect(peeps).to include('New Peep 1!')
+    expect(peeps).to include('Yoyoyo!')
+    expect(peeps).to include('http://www.google.com')


As you're using include, this doesn't actually test that the peeps are in the right order.

siellsiell · 2022-07-20T16:29:15Z

+    <button value="Submit" type="submit">Peep!</button>
+  </form>
+
+  <p><%= @all_peeps.join('/n') %></p>


What's the HTML equivalent of a line break? That's the thing to research.

siellsiell · 2022-07-20T16:31:12Z

+
+describe '.all' do
+  it 'returns all peeps in reverse chronological order' do
+    connection = PG.connect(dbname: 'chitter_test')


Do you actually need this line?

siellsiell · 2022-07-20T16:37:48Z

+    end
+
+    result = connection.exec "SELECT * FROM peeps"
+    return result.map{ |elem| elem["message"]}.reverse


You can actually use the database query to sort the peeps in the right order and save the extra reversal step: https://www.postgresql.org/docs/current/queries-order.html
If you had a lot of peeps, this could improve perfomance quite a lot.

User stories 1 and 2 completed (temporary solution to user story 4)

a69a952

jormark commented Jul 18, 2022

View reviewed changes

siellsiell reviewed Jul 20, 2022

View reviewed changes

Conversation

jormark commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Your name

User stories

README checklist

Uh oh!

jormark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siellsiell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jormark commented Jul 18, 2022 •

edited

Loading