Skip to content

Instantly share code, notes, and snippets.

@seliverstov-maxim
Created August 20, 2019 07:28
Show Gist options
  • Save seliverstov-maxim/dd89709ab5e66af52429763fe35c0365 to your computer and use it in GitHub Desktop.
Save seliverstov-maxim/dd89709ab5e66af52429763fe35c0365 to your computer and use it in GitHub Desktop.

Замечания к ТЗ

  • Почему resources :sessions? сессия же одна мы не можем видеть списка сессий.

  • Странность

     @gists = Gist.opened
        .preload(:user)
        .order(Arel.sql('random()'))
        .limit(10)  
    

    Почему бы не так?

    Gist
      .opened
      .inlcudes(:user)
      .order('RANDOM()')
      .limit(10)
    
  • Зачем использовать респондер? У тебя же нет варианотов других. В некоторых контроллерах это серьезно утяжеляет чтение

    respond_to do |format|
        format.html do
          render layout: 'long_layout'
        end
      end
    
  • Почему?

    return unless current_user.nil?
    

    а не:

    return if current_user.present?
    
  • Зачем в модели user? Это поле есть в secure_passwords

    attribute :password_confirmation, default: ''
    
  • Зачем билдить массив? Мне кажется тем самым ты теряешь информацию о найденных словах (вырывая гисты из search_result). И билдить массив через each не очень когда есть flatten

    class SearchGists
      def self.call(search)
        gists = []
        search_results = PgSearch.multisearch(search[:params])
        search_results.each do |result|
          result.searchable.search_gists.each do |gist|
            gists << gist
          end
        end
        gists
      end
    end
    

    Лично мне кажется это читабельней

    PgSearch
      .multisearch(search[:params])
      .search_results
      .map(&:result)
      .map(&:searchable)
      .map(&:search_gists)
      .flatten
    
  • Чем не устраивает классический хелпер I18n.t?

    <%= i18n_v('search.title') %>
    
  • Какой нить слим или хамл было бы не плохо применить.

  • Что за наркомания? CSS в javascripts? app/assets/javascripts/ckeditor/contents.css

  • В application.css лучше не писать стилей напрямую, тем более когда уже создан файл для кастомных стилей app/ assets/stylesheets/styles/layout/custom.scss. В целом не пижу структуры в организации стилей.

  • Тоже самое и про gist_custom/app/assets/javascripts/application.js Не надо там писать никаких js напрямую.

Мой коллега смотрел тоже:

В целом хорошо Единственное замечание это то, что он вьюшки на erb пишет Если чел с большим опытом рельсы, то наверное он по умолчанию будет на slim делать или как минимум на haml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment