Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

2333-3 #281

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

2333-3 #281

wants to merge 3 commits into from

Conversation

reedzyy
Copy link
Contributor

@reedzyy reedzyy commented Jul 27, 2018

Номер

2333

Номер задания

3

Ссылка на видео с демо

https://youtu.be/3rPRb68eslc

Комментарии

Задание выполнено, но хочу еще добавить некоторых изменений


def prepare_data_for_request
@comments.each_with_index do |comment, index|
@data[:documents] << { id: index.to_s, language: 'ru', text: comment}

Choose a reason for hiding this comment

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

Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

@alexshagov
Copy link
Collaborator

alexshagov commented Jul 30, 2018

God commits are unacceptable. Please, remember it.

erb :add_article
end

post '/articles/add' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Stick to RESTful routes.
e.g.

  • POST '/articles'
  • GET '/article/:id' (as you've done already)


# the class that creates an article
class ArticleBuilder
def initialize(link, key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You pass key through all of the classes. Why?
You could just use it in one class responsible for comments analysis.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants