2015-08-15 7 views
0

A user имеет несколько libraries, и каждая библиотека имеет несколько books. Я хочу знать, есть ли у пользователя книга в одной из его библиотек. Я звоню этот метод с: current_user.has_book?(book):Может ли этот рубиновый метод быть реорганизован?

def has_book?(book) 
    retval = false 
    libraries.each do |l| 
    retval = true if l.books.include?(book) 
    end 
    return retval 
end 

Может ли мой метод быть переработан?

+2

С какой целью? Какова ваша цель в конце? Короче говоря, не обязательно яснее, если ясность - ваша цель. –

+0

Моя цель - принять и понять «советы и подсказки» языка, который я использую для создания более удобного кода и оставаться в соответствии с общим использованием. Я не хочу уродливый код и 10 строк кода, если я могу выполнить задание в 1 или 2. –

+0

В Ruby вы всегда можете заменить символы новой строки точкой с запятой, поэтому вы всегда можете выполнить задание в 1 строке: 'def has_book ? (книга) retval = false; libraries.each do | l | retval = true, если l.books.include? (book) end; return retval конец'. Тем не менее это не обязательно делает его более удобным. (Боковое замечание: я действительно не понимаю одержимость «меньшим количеством линий» или «однострочными».) –

ответ

0

Я не знаю, почему @ Зелёный снял ответ, но это был хороший IMO так что я вставить здесь:

def has_book?(book) 
    libraries.includes(:books) 
      .where(books: {id: book.id}) 
      .any? 
end 
+0

Боковое примечание. Я лично стараюсь избегать вложенных хэшей в 'where', так как они полагаются на имена таблиц. Альтернативой является использование 'merge' так:' .merge (Book.where (id: book.id)) '. Это изменение не имеет смысла как есть, но позволяет вам поменять внутреннее предложение 'where'-scope для области на классе« Книга ». –

0

Это выглядит наиболее Минимизированный версию, я не могу видеть способ Минимизировать его больше:

def has_book?(a)c=false;libraries.each{|b|c=true if b.books.include?a};c end 
1
def has_book?(book) 
    libraries.map { |l| l.books.include?(book) }.any? 
end 

map превращает коллекцию объектов библиотеки в коллекции BOOLS, в зависимости включают ли они книги, и any? возвращается true, если какой-либо из элементов в массиве true.

У этого меньше линий, но ваше исходное решение может быть более эффективным, если вы вернете true, как только найдете библиотеку, содержащую книгу.

def has_book?(book) 
    libraries.each do |l| 
    return true if l.books.include?(book) 
    end 
    return false 
end 

кстати. оба php и ruby ​​появились примерно в то же время.

+1

Ваш первый фрагмент не нужен 'map' вообще: вы можете использовать только' collection.any? {| element | element.whatever? } 'Это так же эффективно, как и последнее, так как оно ломается при первом« истинном »столкновении. –

+0

хорошо пункт. спасибо, что исправил меня. то 'any? 'с блоком - лучший способ решить эту проблему. – rodic

2
def has_book?(book) 
    libraries.any?{|lib| lib.books.include?book} 
end 

Довольно простое, что я могу себе представить. Не Нил безопасный, хотя.

Смежные вопросы