Програмиране с Ruby

Курс във Факултета по Математика и Информатика към СУ

Решение на Първа задача от Христо Хърсев

Обратно към всички решения

Към профила на Христо Хърсев

Резултати

  • 6 точки от тестове
  • 0 бонус точки
  • 6 точки общо
  • 13 успешни тест(а)
  • 1 неуспешни тест(а)

Код

class Integer
def prime?
'1' * abs =~ /^1?$|^(11+?)\1+$/ ? false : true
end
def min_prime_divisor
1.upto(abs).find { |n| n.prime? and remainder(n).zero? }
end
def prime_factors
return [] if abs == 1
[abs.min_prime_divisor] + (abs / abs.min_prime_divisor).prime_factors
end
def harmonic
(1..abs).reduce { |sum, n| sum += 1 / n.to_r }.to_r
end
def digits
abs.to_s.chars.map(&:to_i)
end
end
class Array
def frequencies
reduce({}) { |freq, num| freq.update(num => (count num)) }
end
def average
reduce(:+).to_f / size.to_f
end
def drop_every(n)
1.upto(size).map { |i| self[i - 1] if i.remainder(n).nonzero? }.compact
end
def combine_with(other)
empty? ? other : [first] + other.combine_with(self[1..-1])
end
end

Лог от изпълнението

F.............

Failures:

  1) Integer#prime? checks if a number is prime
     Failure/Error: -13.prime?.should eq false
       
       expected: false
            got: true
       
       (compared using ==)
     # /tmp/d20131023-4395-1vvkc1f/spec.rb:3: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.02346 seconds
14 examples, 1 failure

Failed examples:

rspec /tmp/d20131023-4395-1vvkc1f/spec.rb:2 # Integer#prime? checks if a number is prime

История (3 версии и 3 коментара)

Христо обнови решението на 13.10.2013 08:59 (преди над 10 години)

+class Integer
+ def prime?
+ '1' * abs =~ /^1?$|^(11+?)\1+$/ ? false : true
+ end
+
+ def least_prime_divisor
+ abs == 1 ? 1 : (1..abs).find { |n| n.prime? and self % n == 0 }
+ end
+
+ def prime_factors
+ return [] if abs == 1
+ [abs.least_prime_divisor] + (abs / abs.least_prime_divisor).prime_factors
+ end
+
+ def harmonic
+ (1..abs).inject { |sum, n| sum += 1 / n.to_r }.to_r
+ end
+
+ def digits
+ abs.to_s.scan(/./).map { |digit| digit.to_i }
+ end
+end
+
+
+class Array
+ def frequencies
+ hash = {}.tap{ |hash| map { |number| hash[number] = count number } }
+ end
+
+ def average
+ (inject :+).to_f / length.to_f
+ end
+
+ def drop_every(n)
+ 1.upto(length).each.map { |index| self[index-1] if index % n != 0 } - [nil]
+ end
+
+ def combine_with(other)
+ result = []
+ len = length > other.length ? length : other.length
+ 0.upto(len).each.map { |index| result << self[index] << other[index] }
+ result - [nil]
+ end
+end

Христо обнови решението на 13.10.2013 16:10 (преди над 10 години)

class Integer
def prime?
'1' * abs =~ /^1?$|^(11+?)\1+$/ ? false : true
end
def least_prime_divisor
abs == 1 ? 1 : (1..abs).find { |n| n.prime? and self % n == 0 }
end
def prime_factors
return [] if abs == 1
[abs.least_prime_divisor] + (abs / abs.least_prime_divisor).prime_factors
end
def harmonic
(1..abs).inject { |sum, n| sum += 1 / n.to_r }.to_r
end
def digits
abs.to_s.scan(/./).map { |digit| digit.to_i }
end
end
class Array
def frequencies
- hash = {}.tap{ |hash| map { |number| hash[number] = count number } }
+ hash = {}.tap { |hash| map { |number| hash[number] = count number } }
end
def average
(inject :+).to_f / length.to_f
end
def drop_every(n)
- 1.upto(length).each.map { |index| self[index-1] if index % n != 0 } - [nil]
+ 1.upto(length).map { |index| self[index-1] if index % n != 0 }.compact
end
def combine_with(other)
- result = []
- len = length > other.length ? length : other.length
- 0.upto(len).each.map { |index| result << self[index] << other[index] }
- result - [nil]
+ insert(length + other.length, nil).zip(other).flatten.compact
end
-end
+end

I see what u did there

А ето и малко бележки:

  • least_prime_divisor не е много добро име; smallest_prime_divisor е по-добро
  • В същия този метод, тернарният оператор е лош избор; изразът е твърде сложен за него и става нечетим; по-добре ползвай return 1 if abs == 1
  • Предпочитаме вариантите с думи на проверката за делимост, т.е. remainder(number) и методите zero? и nonzero?
  • Ред 20 - виж метода String#chars
  • На ред 27, няма нужда от това присвояване в началото и тази променлива hash; ако искаш да ползваш tap, окей, но поне го направи на два реда, че така вложеният map не се чете добре;
  • Пак там, вътрешният hash не е добро име; frequencies е по-добро
  • Ред 31 - не се слагат така скобите около inject; по-скоро така: inject(:+).to_f
  • Методите inject и reduce са синоними и предпочитаме reduce, по-близък е до идеята на това, което искаме да постигнем
  • Ред 35 - интервал около -
  • Ред 39 - мутираш текущия обект, това е зло! Пренапиши го

Христо обнови решението на 13.10.2013 21:05 (преди над 10 години)

class Integer
def prime?
'1' * abs =~ /^1?$|^(11+?)\1+$/ ? false : true
end
- def least_prime_divisor
- abs == 1 ? 1 : (1..abs).find { |n| n.prime? and self % n == 0 }
+ def min_prime_divisor
+ 1.upto(abs).find { |n| n.prime? and remainder(n).zero? }
end
def prime_factors
return [] if abs == 1
- [abs.least_prime_divisor] + (abs / abs.least_prime_divisor).prime_factors
+ [abs.min_prime_divisor] + (abs / abs.min_prime_divisor).prime_factors
end
def harmonic
- (1..abs).inject { |sum, n| sum += 1 / n.to_r }.to_r
+ (1..abs).reduce { |sum, n| sum += 1 / n.to_r }.to_r
end
def digits
- abs.to_s.scan(/./).map { |digit| digit.to_i }
+ abs.to_s.chars.map(&:to_i)
end
end
class Array
def frequencies
- hash = {}.tap { |hash| map { |number| hash[number] = count number } }
+ reduce({}) { |freq, num| freq.update(num => (count num)) }
end
def average
- (inject :+).to_f / length.to_f
+ reduce(:+).to_f / size.to_f
end
def drop_every(n)
- 1.upto(length).map { |index| self[index-1] if index % n != 0 }.compact
+ 1.upto(size).map { |i| self[i - 1] if i.remainder(n).nonzero? }.compact
end
def combine_with(other)
- insert(length + other.length, nil).zip(other).flatten.compact
+ empty? ? other : [first] + other.combine_with(self[1..-1])
end
end

Мерси за забележките :) combine_with, беше наистина ужасен.

Видях и сега в style guide-а за size over length и reduce over inject и т.н.

Попринцип гледах да се съобразя с всичко написано, но както се вижда имам желание да са one-liner методчета - just for the sake of greatness.

И възникнаха 2 въпроса:

1) 35-ти ред ползването на i наместо index, ок ли е? Защото оставам с впечетлението, че е практика в Ruby community-то, за разлика от Python таковата (откъдето може да се каже че идвам).

2) Ако отговора на (1) е, че не е добре, комбинирано с неодобрения за име на метод "min_prime_divisor", да спра да се правя на интересен и да пренапиша кода малко по-нормално освен :)

П.П. Sorry за pull request-а, въобще не прегледах closed-натите какво представляват.

i за индекс променлива е окей. Достатъчно универсално разпознаваемо е. Другото може да го оставиш така за разнообразие, ако смяташ, че ще работи :)

Принципно, обаче, човек не трябва да се "прави на интересен", умен, гениален и прочее, когато пише код, особено когато кодът е споделен в екип или е по реален проект и не е просто някакво упражнение. Колкото по-прост и четим е кодът, толкова по-добре.

За pull request-а няма проблем, разбира се.