Skip to content

Add processing of multiple picture#4

Open
vvmarulin wants to merge 1 commit into
artempyanykh:masterfrom
vvmarulin:addMultiplePicture
Open

Add processing of multiple picture#4
vvmarulin wants to merge 1 commit into
artempyanykh:masterfrom
vvmarulin:addMultiplePicture

Conversation

@vvmarulin

Copy link
Copy Markdown

Данная доработка это первое, что я писал на руби. Поэтому хотелось бы оставить некоторые комментарии по поводу того, что я сделал и почему я так сделал. Также, как я знаю, для каждого языка есть некоторый набор правил программирования или советов программирования (best practice), в том числе и для руби, которые я пока не знаю в силу отсутствия опыта.

Теперь по делу.

  1. Символ ":picture" переименовал во множественное число. Если это не делать, то, как мне кажется, при дальнейшем изпользовании такой обёртки будет сбивать с толку тот факт, что ты запрашиваешь изображение, а получаешь список.
  2. Если поля с картинками нет в YML, то возвращаю пустой массив. Можно было и nil оставить, но мне показалось как-то логичнее пустой список. Также пустой массива возвращают и в библиотеках питона при похожих действиях.
  3. Добавил функцию для получения массива всех текстов по xpath'у. Не вижу смысла пытаться объединить её с функцией получения только первого текста. Либо добавлять ещё один параметр у функции, а также добавлять кучу if, что усложнит читаемость. Либо пользоваться везде получением всех текстов по данном xpath'у, а затем выбора нужного, что добавит кучу лишних операций.
  4. Исправил тесты при сравнении рисунков разобранного YML. На основе этого проверяется получается ли пустой массив при отсутствие рисунков, получается ли все рисунки, и вообще является ли эта переменная массивом.
  5. Добавлять валидацию рисунков в классе Offer не вижу смысла, так как кроме проверки того, что получен массив и корректности url, которая в других местах не была реализованна, больше и не знаю что проверять. Только если то, что ссылок на рисунки не больше 10.
  6. Наверное это было лишним, но все-таки добавил. Так как массива до этого нигде не было, то в классе Element нужно было что-то изменять. Добавлять новый тип :array_strings глупо, так как, в случае появления необходимости в обработке массивов флоатов или еще чего-нибудь, наплодилось бы куча типов, а следовательно куча однообразного кода. Поэтому я решил, пусть информация о том, массив это или нет хранится в файле xml.rb, а в классе element.rb будем указывать только тип самого значения. Поэтому я добавил функцию, которая определяет массив данных или одно данное и уже к нему применяет переданную функцию.

Пожалуй это все. Извиняюсь за довольно большой комментарий

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.

1 participant