Решение на Втора задача от Георги Костов
Резултати
- 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 (преди около 10 години)
Георги обнови решението на 26.10.2014 01:10 (преди около 10 години)
Георги, коментари, бележки, напътствия, съвети, подредени не непременно по важност:
Проблеми с конвенциите
Като цяло си се придържал към конвенциите, споменати в нашето ръководство по стил, но на някои места имаш отклонения.
Например:
- Не трябва да има празен ред веднага след началото на дефиниция на
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к символа за коментар. Това е ако не рекорд, то поне в челната тройка :) Надявам се да ти е полезно.
Благодаря за подробния коментар. Скоро ще кача подобрено решение. :)