Skip to content

Commit

Permalink
Replacing ordered parameters with named parameters
Browse files Browse the repository at this point in the history
Given that the first parameter (e.g. `ids`) is an Array, I can see it
being rather easy to instead pass `build_presenters('1', '2', '3')`
which in turn would mean `ids == '1'`, `klass == '2'`, and `args == '3'`

By shifting to named parameters, I hope this clears up the likelihood
of that confusion and unexpected behavior.
  • Loading branch information
jeremyf committed Apr 8, 2017
1 parent 03e658e commit 0afd316
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 11 deletions.
21 changes: 16 additions & 5 deletions app/presenters/hyrax/presenter_factory.rb
Expand Up @@ -3,20 +3,31 @@ module Hyrax
# @note The given IDs are loaded from SOLR
class PresenterFactory
class << self
# @param [Array] args any other arguments to pass to the presenters
# @param [Array] ids the list of ids to load
# @param [Class] presenter_class the class of presenter to make
# @return [Array] presenters for the documents in order of the ids (as given)
def build_for(ids:, presenter_class:, args: [])
new(ids, presenter_class, *args)
end

# @param [Array] ids the list of ids to load
# @param [Class] klass the class of presenter to make
# @param [Array] args any other arguments to pass to the presenters
# @return [Array] presenters for the documents in order of the ids
def build_presenters(ids, klass, *args)
new(ids, klass, *args).build
build_for(ids: ids, presenter_class: klass, args: args)
end
deprecation_deprecate build_presenters: "use .build_for instead"
end

attr_reader :ids, :klass, :args
attr_reader :ids, :presenter_class, :args
alias klass presenter_class
deprecation_deprecate klass: "use presenter_class instead"

def initialize(ids, klass, *args)
def initialize(ids, presenter_class, *args)
@ids = ids
@klass = klass
@presenter_class = presenter_class
@args = args
end

Expand All @@ -25,7 +36,7 @@ def build
docs = load_docs
ids.map do |id|
solr_doc = docs.find { |doc| doc.id == id }
klass.new(solr_doc, *args) if solr_doc
presenter_class.new(solr_doc, *args) if solr_doc
end.compact
end

Expand Down
11 changes: 5 additions & 6 deletions spec/presenters/hyrax/presenter_factory_spec.rb
@@ -1,7 +1,7 @@
require 'spec_helper'

RSpec.describe Hyrax::PresenterFactory do
describe "#build_presenters" do
describe "#build_for" do
let(:presenter_class) { Hyrax::FileSetPresenter }

before do
Expand All @@ -10,7 +10,7 @@
.and_return('response' => { 'docs' => results })
end

subject { described_class.build_presenters(['12', '13'], presenter_class, nil) }
subject { described_class.build_for(ids: ['12', '13'], presenter_class: presenter_class) }

context "when some ids are found in solr" do
let(:results) { [{ "id" => "12" }, { "id" => "13" }] }
Expand Down Expand Up @@ -38,10 +38,9 @@ def initialize(_one, two, three)
end
let(:results) { [{ "id" => "12" }, { "id" => "13" }] }
subject do
described_class.build_presenters(['12', '13'],
presenter_class,
'more',
'and more')
described_class.build_for(ids: ['12', '13'],
presenter_class: presenter_class,
args: ['more', 'and more'])
end
it 'passes all the arguments' do
expect(subject.first.two).to eq 'more'
Expand Down

0 comments on commit 0afd316

Please sign in to comment.