mur-wtag/01_persenter.md

In view we are uing Presenter like this:

# app/views/galleries/show.html

<% present(object: @gallery, shop: @shop, gallery_items: @gallery_items) do |presenter| %>
  <div class="shop-name"><%= presenter.shop_name %></div>
    <div class="post-filter btn-filter d-inline-block">
     <div class="dropdown">
       <a class="filter-dropdown dropdown-toggle" href="#" role="button" id="postFilter" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
         <%= presenter.active_filter.present? ? presenter.active_filter[:name] : 'All Photos' %> <%= embedded_svg 'icon-down-arrow.svg', class: 'icon' %>
       </a>
       <div class="dropdown-menu" aria-labelledby="postFilter">
         <a class="dropdown-item <%= 'active' unless params[:view] %>" href="<%= gallery_path(@gallery, channel: params[:channel]) %>">All Photos</a>
         <% presenter.view_filters.each do |filter| %>
           <%= link_to(filter.path, class: filter.css_classes) do %>
             <%= filter.name %>
           <% end %>
         <% end %>
       </div>
     </div>
  </div>
<% end %>

And we are accessing presenter, through helper module:

# app/helpers/application_helper.rb

def present(klass = nil, object:, **objects)
  klass ||= "#{object.class}Presenter".constantize
  presenter = klass.new(object, self) # we are passing application helper object, so view context goes along with it
  presenter.define_objects(objects)

  yield(presenter) if block_given? # passing ruby code block and yielding modifying it with `presenter` object

  presenter
end

And gallery presenter look like this:

# app/presenters/application_presenter.rb

# frozen_string_literal: true
class ApplicationPresenter
  def initialize(object, template)
    @object = object
    @template = template # view templete
  end

  def define_objects(objects_hash)
    objects_hash.each do |name, obj|
      define_singleton_method(name) do
        obj
      end
    end
  end

  private

  def self.presents(name)
    define_method(name) do
      @object
    end
  end

  def h
    @template
  end
end

# app/presenters/gallery_presenter.rb

class GalleryPresenter < ApplicationPresenter
  presents :gallery

  def view_filters
    @filters = VIEW_FILTERS
    @filters.map do |filter|
      filter = filter.dup
      css_classes = 'dropdown-item'
      css_classes += ' active' if h.params[:view] == filter[:param]
      filter.store(:css_classes, css_classes)
      filter.store(:path, h.gallery_path(gallery, view: filter[:param], channel: h.params[:channel])) # using view template object

      Hashie::Mash.new(filter)
    end
  end

  def active_filter
    view_filters.find { |filter| h.params[:view] == filter[:param] }
  end

  def newest_gallery
    @newest_gallery ||= shop.galleries.order(:updated_at).last # using other object like `shop` which passed from view (from controller).
  end
  
  def shop_name
    shop.name
  end
end

onfido-ruby

It's a very straight-foward, well organised and neat ruby api client! Everything is great, nothing to modify.

What could be better:

1. Should take the API version in config that it could be dynamic. Like now onfido api URI is hardcoded (in lib/onfido/configuration.rb#L46)

Maybe because of some business logic this client only allows API version v3. Still we can refactor it with:

API_VERSION = "v3".freeze
"https://#{region_host}/#{API_VERSION}/"

1. Not sure why here taking arguments as array: lib/onfido/resource.rb#L11. We can simply write it like this:

VALID_HTTP_METHODS.each do |method|
 define_method method do |args|
   make_request(
     method: method, # already it’s symbol
     url: Onfido.endpoint + args.fetch(:path),
     payload: build_query(args.fetch(:payload, {}))
   )
 end
end

1. Need to define paginator when it's listing all records, e.g. <RESOURCE>.all Like in live_photo.all we can write something like this (just a thought):

# lib/onfido/resources/live_photo.rb
...
...
...

def all(applicant_id)
  all_records = []
  paginate("live_photos?applicant_id=#{applicant_id}",
           per_page: 250,
           order_by: :order_by,
           order_direction: :order_direction) do |records|
    all_records << records
  end

  { live_photos: [all_records].flatten }
end

And we can define this paginate method like this:

# lib/onfido/resource.rb
def paginate(path, **args)
  args[:page] = 1
  response = get(path: path, payload: args)
  current_page = response.headers['X-Page'].to_i # yeah right now onfido not responding this header
  total_pages = response.headers['X-Total-Pages'].to_i # yeah right now onfido not responding this header

  yield(response["live_photos"]) # yeah it could be more refactored

  while current_page < total_pages
    args[:page] = current_page + 1
    response = get(path: path, payload: args)

    yield(response["live_photos"])
  end
end

What I liked most:

As I mentioned earlier this library is very well developed. I can mention some points which I loved most:

1. Defining http methods like this: /lib/onfido/resource.rb#L10-L18
1. Error handling is so important for a client and this library handled so perfectly. It’s well organized. They defined ServerError, RequestError classes to handle onfido APIs and separately handled ResClient errors. All errors are handled inside this gem independently parsing remote response or resclient errors.
1. Defined NullLogger (lib/onfido/configuration.rb#L35) - an interface which will not log anything if there is no logger.

lib/postgres-copy/acts_as_copy_target.rb

This code is neat and straight forward too.

1. Found some minor formatting issues which could improve coding cosmetic look but not the functionality (everything is already perfect) at all.

Like:

def copy_to(path = nil, options = {}) # <-- using Parenthesis is a good practice
  options = { delimiter: ",", format: :csv, header: true }.merge(options)

  options_string = "DELIMITER '#{options[:delimiter]}' CSV #{ options[:header] ? 'HEADER' : '' }" # <-- simple refactoring for better readability
  options_string = "BINARY" if options[:format] == :binary # <-- simple refactoring for better readability

  options_query = options.delete(:query) || self.all.to_sql

  if path
    raise "You have to choose between exporting to a file or receiving the lines inside a block" if block_given?
    connection.execute "COPY (#{options_query}) TO '#{sanitize_sql(path)}' WITH #{options_string}"
  else
    connection.raw_connection.copy_data "COPY (#{options_query}) TO STDOUT WITH #{options_string}" do
      while line = connection.raw_connection.get_copy_data do
        yield(line) if block_given?
      end
    end
  end

  self # <-- redundant return
end

1. Just found a tiny room to add option to define importing encoding. I’ve just added couple of simple lines in copy_from method:

def copy_from(path_or_io, options = {}) # <-- using Parenthesis is a good practice
  options = { delimiter: ",", format: :csv, header: true, quote: '"' }.merge(options)
  options[:delimiter] = "\t" if options[:format] == :tsv
  options_string = if options[:format] == :binary
                     "BINARY"
                   else
                     quote = options[:quote] == "'" ? "''" : options[:quote]
                     null = options.key?(:null) ? "NULL '#{options[:null]}'" : nil
                     force_null = options.key?(:force_null) ? "FORCE_NULL(#{options[:force_null].join(',')})" : nil
                     encoding = options.key?(:encoding) ? "ENCODING #{options[:encoding]}" : nil # <--- we can use this to define encoding if `encoding` option is provided
                     delimiter = options[:format] == :tsv ? "E'\t'" : "'#{options[:delimiter]}'"

                     "WITH (" + ["DELIMITER #{delimiter}", "QUOTE '#{quote}'", null, force_null, "FORMAT CSV", encoding].compact.join(', ') + ")" # <-- added encoding option in the query
                   end
  io = path_or_io.instance_of?(String) ? File.open(path_or_io, 'r') : path_or_io

  if options[:format] == :binary
    columns_list = options[:columns] || []
  elsif options[:header]
    line = io.gets
    columns_list = options[:columns] || line.strip.split(options[:delimiter])
  else
    columns_list = options[:columns]
  end

  table = quoted_table_name
  table = connection.quote_table_name(options[:table]) if options[:table]

  columns_list = columns_list.map { |c| options[:map][c.to_s] } if options[:map] # <-- coding styles: space provided
  columns_string = columns_list.size > 0 ? "(\"#{columns_list.join('","')}\")" : ""
  connection.raw_connection.copy_data %{COPY #{table} #{columns_string} FROM STDIN #{options_string}} do
    if options[:format] == :binary
      bytes = 0
      begin
        while line = io.readpartial(10240)
          connection.raw_connection.put_copy_data line
          bytes += line.bytesize
        end
      rescue EOFError => e
        puts e
      end
    else
      line_buffer = ''

      while line = io.gets do
        next if line.strip.size == 0

        line_buffer += line

        # If line is incomplete, get the next line until it terminates
        if line_buffer =~ /\n$/ || line_buffer =~ /\Z/
          if block_given?
            begin
              row = CSV.parse_line(line_buffer.strip, {:col_sep => options[:delimiter]})
              yield(row)
              next if row.all?{|f| f.nil? }
              line_buffer = CSV.generate_line(row, {:col_sep => options[:delimiter]})
            rescue CSV::MalformedCSVError => _e
              next
            end
          end

          connection.raw_connection.put_copy_data(line_buffer)

          # Clear the buffer
          line_buffer = ''
        end
      end
    end
  end
end