When copying code take no more than absolutely necessary

This is part of the Semicolon&Sons Code Diary - consisting of lessons learned on the job. You're in the workflows category.

Last Updated: 2024-11-23

I wanted to validate content_type in Rails (back when it wasn't included) so I coped some code to do so from the internet into my codebase:

(No need to read in detail)

class ContentTypeValidator < ActiveModel::EachValidator
  def validate_each(record, attribute, value)
    return true if !value.attached? || types.empty?

    files = Array.wrap(record.send(attribute))

    errors_options = { authorized_types: types_to_human_format }
    errors_options[:message] = options[:message] if options[:message].present?

    files.each do |file|
      next if valid?(file)

      errors_options[:content_type] = content_type(file)
      record.errors.add(attribute, :content_type_invalid, errors_options)
      break
    end
  end

  private

  def types
    Array.wrap(options[:in]).compact.map do |type|
      Mime[type] || type
    end
  end

  def types_to_human_format
    types.join(', ')
  end

  def content_type(file)
    file.blob.present? && file.blob.content_type
  end

  def valid?(file)
    if options[:with].is_a?(Regexp)
      options[:with].match?(content_type(file).to_s)
    else
      content_type(file).in?(types)
    end
  end
end

Months later, I had a bunch of bugs relating to content_type so I had to revisit this class. Or, seeing as I never read it, rather visit it for the first time. It took me 10 min to debug instead of 1. This is because there were lots of features there that I never used (but nevertheless had to rule out when debugging). Specifically these included: - with option, - the possible types.empty? early return.

Eventually I rewrote a simplified version with more explicit failures:

class ContentTypeValidator < ActiveModel::EachValidator
  def validate_each(record, attribute, value)
    # Clearer failure mode
    raise 'Missing mime types' if types.empty?
    return :no_attachment unless value.attached?

    files = Array.wrap(record.send(attribute))

    errors_options = { authorized_types: types_to_human_format }
    errors_options[:message] = options[:message] if options[:message].present?

    files.each do |file|
      next if valid?(file)

      errors_options[:content_type] = content_type(file)
      record.errors.add(attribute, :content_type_invalid, errors_options)
      break
    end
  end

  private

  def types
    Array.wrap(options[:in]).compact.map do |type|
      Mime[type] || type
    end
  end

  def types_to_human_format
    types.join(', ')
  end

  def content_type(file)
    file.blob.present? && file.blob.content_type
  end

  def valid?(file)
    content_type(file).in?(types)
  end
end

--

The same thing happened to me before when I extracted the autocomplete code from a Ruby gem by copying out the relevant files. However, there were many optional flags still inside the code, leading to tons of confusion and headaches down the road. I should have just extracted the minimal I needed for my codebase on day 1.

Lesson

When copying code, you'd do well to take immediate ownership and understand every line. And to only include the features you need for your minimal case.