Поругайте код

Отступ в 2 пробела - фу.

Это же не пхп, ты что, в Руби иногда и больше двух отступов бывает.

Только \t

Это же не пхп, ты что, в Руби иногда и больше двух отступов бывает.

«Многоэтажность» в коде на императивном языке - фу.

Цель у кода какая? Цикл бредовый какой-то. В запросе явно order не хватает, может в default_scope где, код 100% рассчитан на «order by time asc», да еще и на то что будет максимум одна запись в сутки. Я могу предположить что ты хочешь получить «историю» позиций типа «10 - 15 - 20 - 25» и т.д., в этом случае ты чего-то не то замутил.

«start.to_s(:db) == list[i].time.beginning_of_day.to_s(:db)» - что-то жуткое, «start.to_date == list[i].time.to_date» если хочешь проверить что тот же день.

Если я правильно понимаю че ты хотел - то примерно так это могло бы выглядеть.

def words_pos(domain, word, start, stop)
  positions = Position.where(:domain_id => domain, :word_id => word, :time => start..stop)
                      .group("date(`time`)").order("`time`")
  positions.map(&:position).map(&:to_s).zip(["-"] * (positions.length - 1)).flatten.compact  
end

И да, в коде как таковом у тебя совсем хреново только сравнение дат через строки, идеологически можно придраться еще к ненужному return и формированию условия запроса. Тут алгоритм кривой в нет.

Гм.. твой код делает нечто совершенно неприменимое. Однако, map на map я никогда не видел и это интересно. Покопаюсь :)

return не нужен как лишние 7 символов, разве что :) С ним читаемость выше, имо.

Про сравнение дней понял, спасибо.

Если бы ты хоть написал что ты хотел получить тем кодом. С моей точки зрения - он как раз делает что-то непонятное совершенно.

Мой код тянет с бд максимум одну запись ( group by time ) на каждый день, в порядке возрастания даты ( order by time asc ). Далее у каждой записи берем свойство .position ( первый map ), получаем список position'ов, конвертим в строки .to_s ( второй map).

Единственная магия - .zip, но на самом деле там тоже просто. Допустим у нас было ['1', '2', '3'] в positions после двух .map, .length этого дела - 3, ["-«] * (3-1) => [»-", "-«]. После zip'а будет [[„1“, »-«], [„2“, »-«], [„3“, nil]], nil в конце потому что у нас второй список недостаточно длинный, руби добивает nil'ами в этом случае. .flatten ожидаемо раскладывает это в [„1“, »-", «2», "-", «3», nil]. .compact убирает из массива nil'ы, получаем в итоге [«1», "-", «2», "-", «3»].

Как я понимаю это не то что тебе надо было на самом деле. В твоем коде "-" добавляется в случае если для данной даты нет записи в таблице, если я правильно понял в этом вся соль.

В таком случае можно что-то вроде

def words_pos(domain, word, start, stop)
  positions = Hash[
    Position.select("`time`, `position`")
            .where(:domain_id => domain, :word_id => word, :time => start..stop)
            .group("date(`time`)").order("`time`")
            .map{|p| [p.time.to_date, p.position]}          
  ]
  (start.to_date..stop.to_date).map{|day| positions[day] || "-" }
end

Однако, map на map я никогда не видел

лол

＞Поругайте
＞positions, i = [], 0
Ну вот, собственно… тебе что, переводов строки жалко? Воспользуйся точкой с запятой.

Класс! Есть куда расти.

И да, временный контейнер «positions» не нужен, равно как и «i». Если не трогать логику происходящего, то писать скорее надо так =>

def words_pos(domain, word, start, stop)
  list = Position.where("`domain_id` = ? AND `word_id` = ? AND `time` BETWEEN ? AND ?", domain, word, start, stop)

  (start.to_date...stop.to_date).map do |day|
    if day == list.first.time.to_date
      list.shift.position.to_s
    else
      '-'
    end
  end
end

while с добавлением результатов в контейнер заменить на более лаконичный обход по Range между датами. Ссылка на «текущий» элемент массива «i» не нужна, т.к. ты по факту каждый раз переходишь к следующему элементу массива, старый потом не используются. С тем же успехом можно просто отрезать первый элемент массива когда надо.

Не забываем, что в руби любое выражение, в том числе if имеет implicit возврат, как правило return - признак или хитровыебанного алгоритма или нарушение принципа про монастырь и чужой устав.

Понял. Мне вполне ясно как этот код работает, но привычки укорачивать ещё нет. Где можно почитать про хороший стиль?

Очень плохо, что используется язык, где блоки кода выделяются табуляцией. Если мы и дальше будем выбирать такие языки, эта порочная особенность никогда не исчезнет, что, безусловно, недопустимо.

list = Position.where(«`domain_id` = ? AND `word_id` = ? AND `time` BETWEEN ? AND ?», domain, word, start, stop)

SQL-инъекцию этому пациенту.

Рельсы заэкранируют.

на руби блоки кода отделяются табуляцией? это что-то новенькое