-
Notifications
You must be signed in to change notification settings - Fork 43
2345 - 3 #292
base: master
Are you sure you want to change the base?
2345 - 3 #292
Conversation
2345/3/mechanize_onliner_0.0.1.rb
Outdated
} | ||
# end | ||
|
||
puts JSON.pretty_generate(reviews) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/TrailingBlankLines: Final newline missing.
2345/3/mechanize_onliner_0.0.1.rb
Outdated
reviewer = review_meta.search('h4 address')[0].text | ||
review_date = Date.parse(review_meta.search('.pub-date')[0].text) | ||
score = review_meta.search('.score').text.to_f | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/IndentationConsistency: Inconsistent indentation detected.
Lint/Void: Literal {
2345/3/mechanize_onliner_0.0.1.rb
Outdated
label, year = review_meta.search('h3')[0].text.split(';').map(&:strip) | ||
reviewer = review_meta.search('h4 address')[0].text | ||
review_date = Date.parse(review_meta.search('.pub-date')[0].text) | ||
score = review_meta.search('.score').text.to_f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/IndentationConsistency: Inconsistent indentation detected.
2345/3/mechanize_onliner_0.0.1.rb
Outdated
album = review_meta.search('h2')[0].text | ||
label, year = review_meta.search('h3')[0].text.split(';').map(&:strip) | ||
reviewer = review_meta.search('h4 address')[0].text | ||
review_date = Date.parse(review_meta.search('.pub-date')[0].text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/IndentationConsistency: Inconsistent indentation detected.
2345/3/mechanize_onliner_0.0.1.rb
Outdated
artist = review_meta.search('h1')[0].text | ||
album = review_meta.search('h2')[0].text | ||
label, year = review_meta.search('h3')[0].text.split(';').map(&:strip) | ||
reviewer = review_meta.search('h4 address')[0].text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/IndentationConsistency: Inconsistent indentation detected.
2345/3/mechanize_onliner_0.0.1.rb
Outdated
agent = Mechanize.new | ||
page = agent.get('https://people.onliner.by/2018/07/23/vodol') | ||
|
||
# reviews = review_links.map do |link| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/CommentIndentation: Incorrect indentation detected (column 0 instead of 2).
2345/3/mechanize_onliner_0.0.1.rb
Outdated
require 'mechanize' | ||
|
||
agent = Mechanize.new | ||
page = agent.get('https://people.onliner.by/2018/07/23/vodol') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint/UselessAssignment: Useless assignment to variable - page.
2345/3/Gemfile
Outdated
gem 'sinatra', '~> 1.4', '>= 1.4.7' | ||
gem 'redis', '~> 3.3', '>= 3.3.1' | ||
gem 'ohm', '~> 3.1', '>= 3.1.1' | ||
gem 'mechanize', '~> 2.7', '>= 2.7.6' # https://rubygems.org/gems/mechanize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bundler/OrderedGems: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem mechanize should appear before ohm.
2345/3/Gemfile
Outdated
|
||
gem 'sinatra', '~> 1.4', '>= 1.4.7' | ||
gem 'redis', '~> 3.3', '>= 3.3.1' | ||
gem 'ohm', '~> 3.1', '>= 3.1.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bundler/OrderedGems: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem ohm should appear before redis.
2345/3/Gemfile
Outdated
gem 'rubocop', '~> 0.54.0' | ||
|
||
gem 'sinatra', '~> 1.4', '>= 1.4.7' | ||
gem 'redis', '~> 3.3', '>= 3.3.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bundler/OrderedGems: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem redis should appear before sinatra.
@@ -0,0 +1,13 @@ | |||
module CommentParser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it should have the same structure as ArticleParser
Do you have some reason to use module in this case?
I would do smth like this:
class CommentParser
def initialize(comment_data)
end
def votes
end
def author
end
def text
end
end
@comments = comments | ||
end | ||
|
||
# :reek:FeatureEnvy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is bad practice.
It means that code get too complex
I could show how to fix it - but there are no specs for the class - so I will take much time for me (other developer) to understand what it does and refactor or extend.
If you want - you could add specs for this particular class and I'll show how to fix the 'FeatureEnvy' issue
or you can fix it yourself
@@ -0,0 +1,15 @@ | |||
class CommentsCreater | |||
LIMIT = 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LIMIT = 50 - never used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete it
@@ -0,0 +1,28 @@ | |||
class ArticleCreater | |||
attr_reader :article |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You break Command Query separation principle:
when you say ArticleCreater#create - it is a command method - it COMMANDs to do smth
when you need article - it is a QUERY for a state of the object - so you should have 2 separate method for it
(I use attr_reader to create this method see example below)
so you should split the #create method into 2 methods
class ArticleCreator
attr_reader :options #input
attr_reader :article #output
# helper method not to break command/query separation principle
# you should replace all places where you use ArticleCreater.new(options).create to ArtcleCreator.create(options)
def self.create(options)
article_creator = new(options)
article_creator.create(options)
article_creator.article
end
def initialize(options)
end
def create
create_article
create_comments
end
private
def create_article
@article = Article.create(link: @article_link, title: title, rating: article_rating)
end
def create_comments
CommentsCreater.new(comments_with_rating, article).create
end
end
Номер
2345
Номер задания
3
Ссылка на видео с демо
https://youtu.be/voqGG5hFiGk
Комментарии
Task waiting for review.
Features: