Решение на Втора задача от Георги Костов
Резултати
- 6 точки от тестове
 - 0 бонус точки
 - 6 точки общо
 
- 23 успешни тест(а)
 - 1 неуспешни тест(а)
 
Код
Лог от изпълнението
.......................F
Failures:
  1) NumberSet returns enumerable of set's contents if no block is given to each
     Failure/Error: expect(numbers.each.to_a.size).to eq [1, 3, 5].size
     LocalJumpError:
       no block given (yield)
     # /tmp/d20141028-18133-1hmdbup/solution.rb:40:in `block in each'
     # /tmp/d20141028-18133-1hmdbup/solution.rb:39:in `each'
     # /tmp/d20141028-18133-1hmdbup/solution.rb:39:in `each'
     # /tmp/d20141028-18133-1hmdbup/spec.rb:164:in `block (2 levels) in <top (required)>'
     # ./lib/language/ruby/run_with_timeout.rb:5:in `block (3 levels) in <top (required)>'
     # ./lib/language/ruby/run_with_timeout.rb:5:in `block (2 levels) in <top (required)>'
Finished in 0.02323 seconds
24 examples, 1 failure
Failed examples:
rspec /tmp/d20141028-18133-1hmdbup/spec.rb:159 # NumberSet returns enumerable of set's contents if no block is given to each
История (6 версии и 2 коментара)
Георги обнови решението на 26.10.2014 00:36 (преди около 11 години)
Георги обнови решението на 26.10.2014 01:10 (преди около 11 години)
Георги, коментари, бележки, напътствия, съвети, подредени не непременно по важност:
Проблеми с конвенциите
Като цяло си се придържал към конвенциите, споменати в нашето ръководство по стил, но на някои места имаш отклонения.
Например:
- Не трябва да има празен ред веднага след началото на дефиниция на 
class Foo, както и непосредствено преди затварящияend. - Ред 11 и 12 не са правилно отместени навътре.
 - Дефиницията 
publicна 65 ред трябва да е отместена едно ниво навътре, редом сdef-овете и да има празен ред след нея. Освен това е безсмислена. Всички методи са public по подразбиране. - Имаш проблеми с отместването на 
filtrateметода. - На 87-ред по конвенция се изпуска 
self., защото няма смисъл от него. - Когато дефинираш методи, приемащи аргументи, се слагат кръгли скоби (напр. ред 105 и 110).
 - Хубаво е да има скоби около 
nна редове 106 и 111, защото е малко объркано иначе. 
Дизайн, архитектура, стил, функционалност
- Използвал си т. нар. "monkey patching" - добавяне/промяна на методи в съществуващи класове; в случая, 
Array#unique_valuesи промените вInteger,FloatиRational. Да, правят кода по-четим, но "замърсяват" всички класове с тези методи. Този тип monkey patching е много примамлива техника, но и същевременно нещо, с което много трябва да се внимава. Ако пишеш библиотека, е почти забранено. Ако пишеш твой проект, е донякъде окей (защото си мажеш твоите неща и твоето глобално пространство). Домашните принципно спадат към втората категория, но искаме да ви научим да внимавате с използването на monkey patching. Съвсем накратко, ако съществува достатъчно елегантно решение без monkey patching (каквото в случая - а и почти винаги - има), ще ви караме да не ползвате monkey patching. Така че съветът ми е да го премахнеш. - Допълнително за 
Array#unique_values- сигурен ли си, че няма такава вградена в Ruby функционалност? Провери ли документацията на Array, преди да го напишеш този метод, както сме казвали нееднократно на лекции? - 
Още повече, че по самата имплементация на гореописания метод имам много забележки.
reverseе излишен; ако искаш да махаш елементи от единия край, няма значение кой край ползваш; а и имаArray#shift, което е катоpop, но от началото.kе лошо име на променлива; по-добре би било да се казваelement, например.whileи изобщоpopса излишни. Ето ти примерна имплементация на този метод (забележи идентацията ми и къде има нови редове):class Array def unique_values unique_elements = [] each do |element| unique_elements << element unless unique_elements.include?(element) end unique_elements end endВ Ruby има и клас
Set, който би бил подходящ в случая. Но, пак, виж документацията наArrayи прецени има ли смисъл ти да пишеш такъв метод. attr_writer+attr_reader=attr_accessor- Защо на 25-ти ред кръщаваш променливата си 
@set, когато всъщност тя е списък? Защо не ползваш класаSet? :) - Има ли смисъл от 
returnиifвempty?? Защо не просто@set.size == 0? А може би въпросният обект@setсъщо има методempty?, който би могъл да се ползва? - Ред 37 - какво е 
new?new_number, може би? - Ред 48 е празен и не трябва да го има.
 - В 
eachна ред 47 - почти същите забележки имам като заunique_values- не ползвай еднобуквени имена катоk,l; не ползвайwhile; не правиdupиreverse(още повече -dupби бил излишен, понежеreverseвръща ново копие; което по случайност те е спасило от ужасна грешка вArray#unique_values, но явно е било случайно, понеже не си ползвалdupтам, а тук си го ползвал); не ползвайpop. - Методите 
printне трябва да го има в окончателното ти решение. Ако трябваше да останат, щеше да е добре да ги кръстишto_sилиinspect, каквито са Ruby конвенциите в случая. Даже ако ги кръстишinspect, irb ще ти показва този текст, когато резултатът от израз еNumberSetобект. - 
В метода
NumberSet#[]на ред 58, дизайнът със setter-аinsert=е лош. И защо изобщо се казваinsert? Какво вмъква? Може да подходиш така - методът тиto_aна филтрите ми се струва излишен. Ползваш го само тук. На свой ред,filtrateсе ползва само отto_a. Аз лично мисля, чеfiltrateспокойно може да се казваeachи методътto_aда отпадне. Нещо повече, този методfiltrate(each) може да приема аргумент -- списъкът/нещото, по което да итерира (това, което сега подаваш презinsert=). Така ще може да го ползваш по следния начин:def [](filter) result = NumberSet.new filter.each(@set) { |element| result << element } result end За някои от методите в
SharedMethodsсе разбрахме, че не са нужни. Другите може да ги сложиш в базов клас на филтрите, но тъй като не сме говорили за наследяване все още, и така е окей.- Променливата 
new_filterв методите&и|е напълно излишна. Трябва да се махне. - Няма причина да дефинираш ръчно getter и setter за 
@block. Ползвай клас-макроситеattr_*за целта. Още повече, че да имаш setter за блок не е добра идея. И за капак - не мисля, че някъде изобщо ползваш този setter. Разкарай го. - Методите в модула 
NumberChecksспоред мен не трябва да ги има. Вж. бележката ми за monkey patching. Но ако ги имаше, щеше да е напълно окей да са самоself > 0, например, безif, безreturnи безfalseна втори ред. Ако ползваш тези сравнения директно във филтрите си, няма да изгубиш четимост (нещо < 0ми се струва достатъчно четимо), като същевременно ще спестиш 30-40 реда код и ще избегнеш monkey patching-а. - За да провериш от какъв клас е даден обект, няма нужда да обръщаш името на класа до низ. Може да ползваш само 
foo.class == Complex(например). Но най-добре ползвайfoo.is_a?(Complex). - Аз бих подравнил вертикално 
thenклаузите вcase-а на ред 196. Освен това, може да е@block = case type .... 
Твоето решение ме принуди да напиша 6к символа за коментар. Това е ако не рекорд, то поне в челната тройка :) Надявам се да ти е полезно.
Благодаря за подробния коментар. Скоро ще кача подобрено решение. :)
