start with OOP #5

Gusarov2k · 2020-05-28T13:38:18Z

No description provided.

zverok · 2020-05-29T13:08:03Z

movie.rb

+  attr_accessor :link, :original_title, :year, :country, :release_date, :genres, :runtime, :popularity, :film_director,
+                :stars
+
+  def initialize(params = {})


Лучше использовать keyword arguments.
Hash of params это очень старый трюк, в современном руби к нему лучше не прибегать.

zverok · 2020-05-29T13:09:45Z

movie.rb

-  movies.each do |movie|
-    puts "#{movie.original_title} (#{movie.release_date.strftime('%d - %B - %Y')} " \
-         "#{movie.genres}) - #{movie.runtime}"
+  def has_genre?(param)


Лучше стараться аргументы называть конкретно по смыслу, а не «аргумент», «параметр» и т.д.

zverok · 2020-05-29T13:12:43Z

movie_collection.rb

+
+class MovieCollection < Movie
+  require 'date'
+  require 'csv'


require лучше оставить снаружи класса (в начале файла), а то оно выглядит не так как ведёт себя (как будто включит CSV только внутрь класса, но это не так)

zverok · 2020-05-29T13:13:38Z

movie_collection.rb

+  attr_accessor :movies
+
+  def initialize(file_name)
+    input_arg = ARGV[1]


Нет, если мы пишем библиотечный класс, лучше ему не работать с ARGV (пусть это делает скрипт его использующий), иначе его будет неудобно использовать.
Пусть просто принимает имя файла который открыть (и держит «имя файла по умолчанию» в константе)

zverok · 2020-05-29T13:14:37Z

movie_collection.rb

+    CSV.read(file_name, col_sep: '|',
+                        headers: %i[link original_title year country release_date
+                                    genres runtime popularity film_director stars],
+                        converters: %i[date integer float]).map { |row| @movies.push(Movie.new(row.to_hash)) }


нет, push не нужен тут :) надо подумать как тут правильно использоать map.
И кстати список заголовков (да и список конвертеров) можно вынести в костанты, тогда код будет легче читать.

zverok · 2020-05-29T13:18:05Z

movie_collection.rb

+
+  def sort_by(param)
+    if movies.first.send(param).is_a? Date
+      films = movies.select { |val| val.send(param).is_a? Date }


На самом деле это не очень хороший ход — а если как раз у первого фильма дата не распарсилась?
Я бы скорее усложнил немного парсинг даты в фильмах (там же её не то чтобы нету, просто у некоторых она только годом, её вполне можно превратить в «1 января того года» просто)

zverok · 2020-05-29T13:18:29Z

movie_collection.rb

+      films = movies.select { |val| val.send(param).is_a? Date }
+      films.sort_by { |val| val.send(param) }
+    else
+      movies.sort_by { |val| val.send(param) }


sort_by(&param) можно просто.
кстати, что сделает этот код при сортировке по длительности фильма?

zverok · 2020-05-29T13:20:20Z

movie_collection.rb

+  end
+
+  def filter(param)
+    if param.keys.first == :genres || param.keys.first == :country


Не, должно со всеми полями работать :)
И кроме того:

работать когда фильтров несколько: filter(year: 2001, country: 'USA')

работать с паттернами, например: filter(year: 2000..2005, title: /terminator/i) (тут пригодится метод ===)

zverok · 2020-05-29T13:20:44Z

movie_collection.rb

+  end
+
+  def stats(param)
+    movies.group_by { |movie| movie.send(param) }.transform_values(&:count).sort_by(&:last)


А что будет при попытке посчитать статистику по актёрам, или по жанрам? ;)

↑ На этот вопрос по-моему всё ещё нету ответа :)

zverok · 2020-06-02T12:51:18Z

.rubocop.yml

@@ -3,3 +3,12 @@ Metrics/LineLength:
  StyleGuide: '#80-character-limits'
  Enabled: true
  Max: 120
+
+Metrics/MethodLength:
+  Max: 40


Это очень много :)

zverok · 2020-06-02T12:52:04Z

.rubocop.yml

+  Max: 40
+
+Metrics/AbcSize:
+  Max: 50


Это очень-очень много. Для учебного кода 20 — это в общем норм, больше не стоит. Если метод становится слишком сложным, надо его упрощать и/или разбивать на другие методы.

zverok · 2020-06-02T12:54:20Z

movie.rb

+    @runtime = args[:runtime]
+    @popularity = args[:popularity]
+    @film_director = args[:film_director]
+    @stars = args[:stars]


Если мы знаем, какие у метода будут аргументы с ключами, это имеет смысл так и описать (keyword arguments):

def initialize(link:, original_title:, year:...)

Во-первых, код станет проще внутри метода, во-вторых по описанию метода сразу будет видно чего он ожидает, в-третьих, станет невозможно его вызвать неправильно (с не теми аргументами)

zverok · 2020-06-02T13:04:31Z

movie_collection.rb

+
+  HEADERS_KEYS = %i[link original_title year country release_date genres runtime popularity film_director stars].freeze
+  CONVERTERS_KEYS = %i[date integer float].freeze
+  INPUT_ARG = ARGV[1]


Нет, это неверно :) Константа — это константа, не то что имеет право меняться при каждом запуске.

zverok · 2020-06-02T13:05:01Z

movie_collection.rb

+                    val.release_date = Date.parse(val.release_date + '-01')
+                  elsif val.release_date.is_a? Integer
+                    val.release_date = Date.new(val.release_date)
+                  end


Эту логику (как правильно хранить дату) должен взять на себя фильм, коллекции для этого никакого дела нету.

zverok · 2020-06-02T13:06:35Z

movie_collection.rb

+  def stats(param)
+    special_keys = %i[genres stars]
+    group = if special_keys.include? param
+              movies.group_by { |movie| movie.send(param).split(',').shift }


Хранить поля, для которых это релевантно, в виде массивов — это тоже дело фильма. У коллекции тут должен быть очень простой и обобщённый код.

zverok · 2020-06-04T12:11:28Z

movie.rb

+                    elsif release_date.is_a? Integer
+                      Date.new(release_date)
+                    end
+    @genres = genres.split(',').shift


а shift зачем?..

zverok · 2020-06-04T12:12:50Z

movie.rb

-end
+  def actors
+    stars.split(',')
+  end


а зачем этот метод вообще? (если зачем-то таки нужен — можно решить через alias просто... или переименовать колонку исходную :))

↑ Этот комментарий пропущен.

zverok · 2020-06-04T12:15:25Z

movie.rb

-                                              genres runtime popularity film_director stars],
-                                  converters: %i[date integer float])
-                 .map { |row| OpenStruct.new(row.to_hash) }
+  def genre?(movies, param)


Это не соответствует заданию :) «Клиент» этого кода не должен думать что туда передать — он просто спрашивает movie.genre?('Drama')
Чтобы у фильма была возможность узнать какие жанры вообще бывают, имеет смысл чтобы коллекция, при конструировании фильма, передавала ему себя — тогда фильм может хранить в своей переменной ссылку на всю коллекцию, и при необходимости спрашивать у неё «какие вообще жанры существуют?»

zverok · 2020-06-04T12:22:22Z

movie_collection.rb

+        else
+          params[key].member? d.send(key)
+        end
+      end


Вместо params.keys.all do |key| и потом использовать param[key], мы можем использовать params.all? do |key, value|

Логика тут запуталась. param[key].is_a?(Array) — какой случай это поддерживает, какой код? Работает ли сейчас filter(title: /terminator/i)? И т.д. Правильная логика — а) использовать === для сравнения (тогда и регулярные выражения, и диапазоны, и простые строки будут работать) и б) нам нужны всего две ветки if — для полей в которых массив (типа stars — stars: /Schwarzeneg/ тоже должно работать), и для всех остальных полей.

А ещё лучше вынести проверку фильтра в фильм — метод выглядящий как movie.matches?(field, pattern) — тогда в коллекции будет совсем простой код, а в фильме он тоже будет довольно несложный и его легко будет протестировать.

zverok · 2020-06-04T12:22:42Z

movie_collection.rb

+  end
+
+  def stats(param)
+    movies.group_by { |movie| movie.send(param) }.transform_values(&:count).sort_by(&:last)


↑ На этот вопрос по-моему всё ещё нету ответа :)

zverok · 2020-06-05T11:35:13Z

.rubocop.yml

+  Max: 20
+
+Metrics/ParameterLists:
+  Max: 11


Лучше CountKeywordArgs: false (тогда он не будет ругаться ТОЛЬКО на методы с keyword args, что логично — их можно и две дюжины сделать без потери понятности)

zverok · 2020-06-05T11:39:09Z

demo.rb

+movies = MovieCollection.new('movies.txt')
+movies.sort_by(:release_date)
+
+movies.filter(country: 'USA', year: 2001..2002, runtime: '92 min')


К слову, лучше бы runtime тоже хранить числом, потому что более-менее реальное применение этого фильтра — что-то вроде «хочу посмотреть фильм не больше 1.5 часов, т.е. runtime: 60..90 (а «хочу посмотреть фильм длиной 92 минуты» — не очень реальное :))

zverok · 2020-06-05T11:42:37Z

movie_collection.rb

+  end
+
+  def filter(params)
+    movies.select { |d| params.all? { |key, _| params[key] === d.send(key) } }


_ — это как раз то значение, которое params[key], тут вы сами себе жизнь усложняете :)
А работает ли этот код с жанрами? И скажем с актёрами?

zverok · 2020-06-05T11:44:42Z

movie_collection.rb

+      movies.map { |movie| movie.send(param) }.flatten.group_by(&:itself).transform_values(&:count).sort_by(&:last)
+    else
+      movies.group_by { |movie| movie.send(param) }.transform_values(&:count).sort_by(&:last)
+    end


Вместо двух веток, во многих случаях можно просто обобщить код. Например, flat_map(&param).group_by(&:itself) сработает для всех вариантов содержимого поля.

zverok

уже близко :)

zverok · 2020-06-09T13:33:43Z

movie.rb

-end
+  def actors
+    stars.split(',')
+  end


↑ Этот комментарий пропущен.

zverok · 2020-06-09T13:37:24Z

movie_collection.rb

+                                  headers: HEADERS_KEYS,
+                                  converters: CONVERTERS_KEYS).map do |row|
+      row[:all_movies] = self
+      Movie.new(row.to_hash)


Можно, во-первых, вот так:

Movie.new(all_movies: self, **row.to_hash)

(** это «распаковать keyword args из этого хеша»)
а во-вторых лучше всего сделать коллекцию единственным НЕ-keyword аргументом, чтобы сразу видна была разница:

# в Movie def initialize(collection, link:, original_title: ....) # вызов в коллекции: Movie.new(self, row.to_h)

добавление методов change methods change methods change methods changes change method refactoring

zverok reviewed May 29, 2020

View reviewed changes

zverok reviewed Jun 2, 2020

View reviewed changes

zverok reviewed Jun 4, 2020

View reviewed changes

zverok reviewed Jun 5, 2020

View reviewed changes

Gusarov2k force-pushed the Task_6 branch from c426432 to 64309a4 Compare June 9, 2020 13:09

zverok reviewed Jun 9, 2020

View reviewed changes

start with OOP

c297606

добавление методов change methods change methods change methods changes change method refactoring

Gusarov2k force-pushed the Task_6 branch from 64309a4 to c297606 Compare June 9, 2020 14:27

Gusarov2k merged commit 5f16df9 into master Jun 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start with OOP #5

start with OOP #5

Gusarov2k commented May 28, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok May 29, 2020

zverok Jun 4, 2020

zverok Jun 2, 2020

zverok Jun 2, 2020

zverok Jun 2, 2020

zverok Jun 2, 2020

zverok Jun 2, 2020

zverok Jun 2, 2020

zverok Jun 4, 2020

zverok Jun 4, 2020 •

edited

Loading

zverok Jun 9, 2020

zverok Jun 4, 2020

zverok Jun 4, 2020

zverok Jun 4, 2020

zverok Jun 5, 2020

zverok Jun 5, 2020

zverok Jun 5, 2020

zverok Jun 5, 2020

zverok left a comment

zverok Jun 9, 2020

zverok Jun 9, 2020

start with OOP #5

start with OOP #5

Conversation

Gusarov2k commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zverok Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zverok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zverok Jun 4, 2020 •

edited

Loading