Feedback for https://github.com/lagoan/era_audit

era_audit.md

Running rubocop can show where this code strays from best practices/community standards. It doesn't make sense to make these changes to the code, we've already talked about the code being somewhat disposable, but could be informative to know about.

• Style/StringConcatenation: Prefer string interpolation to string concatenation.

For example

# rails/generate_report_12.rb:28:110
missing_community_collection_report[:community] << "Community Id %{community_id} does not exist" % {community_id: community_collection[0]}          

becomes "Community Id #{community_collection[0]} does not exist"

# rails/generate_report_1.rb:11:15
file_name = root_directory + '/report_1_' + entity_type + '_with_metadata_only_' + Time.now.to_formatted_s(:number) + '.csv'

becomes "#{root_directory}/report_1_#{entity_type}_with_metadata_only_#{Time.now.to_formatted_s(:number)}.csv"

• Style/NumericPredicate: Use entity.files.count.zero? instead of entity.files.count == 0. Another way to say the same thing is files.attached? era_audit/rails/generate_report_1.rb:14:53
• Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
• Style/IfUnlessModifier: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||. For example

# rails/generate_report_13.rb:14:5
Community.find_each do |community|
  unless community.description.present?
    csv << [community.title, get_community_url(community)]
  end
end

becomes csv << [community.title, get_community_url(community)] unless community.description.present? but it would be more efficient to reduce the problem space earlier with a where clause like this

Community.where(description: [nil, '']).find_each do |community|
  csv << [community.title, get_community_url(community)]  
end 

• Style/GlobalVars: Do not introduce global variables. For example

# rails/generate_report_5.rb:5:1
$custom_error_report = {custom: {}, oai: {}}

This is a bit more complicated, you have to think about what the methods are doing and returning. I realize this is my preference but another way to do this would be to drop the is_entity_valid? method push this logic to where it's used. For cleanup use compact_blank to retun a hash without the blank values. And either infer the valid? part from the error hash, or use parallel assignment to return two values [boolean, Hash].

• Naming/PredicateName For example

# rails/generate_report_5.rb:7:5
def is_entity_valid?(entity)

becomes def entity_valid?(entity)

• Style/ClassEqualityComparison: Use instance_of?(Thesis) instead of comparing classes. For example

# rails/generate_report_5.rb:14:16
is_valid = if entity.class == Item
    is_item_valid?(entity)
  elsif entity.class == Thesis
    is_thesis_valid?(entity)
  end

becomes entity.instance_of(Item) or use a case statement to simplify further

is_valid = case entity
when Item
  is_item_valid?(entity)
when Thesis
  is_thesis_valid?(entity)
end

• Style/ZeroLengthPredicate: Use empty? instead of length == 0.
• Style/MethodCallWithoutArgsParentheses: Do not use parentheses for method calls with no arguments.

This occurred to me when I looked at the code before but I couldn't remember what it was called. In the first four reports there is a pattern like this:

[Item, Thesis].each do |klass|
  klass.find_each do |entity|
    entity.files.each do |file|
      # do the thing
    end
  end
end

One way to make this more efficient (this is an example of the N+1 query pattern) is to eager load associations. It would look something like this using includes

[Item, Thesis].each do |klass|
  klass.includes(files_attachments: :blob).find_each do |entity|
    entity.files.each do |file|
      # do the thing
    end
  end
end

Thanks for this @pgwillia