Ruby format analyser

Question

I have a requirement to validate file names related to architecture after they are uploaded. Once they have been uploaded I must warn the user if the file name is not standards compliant.

What's in a name

To be standards compliant a file name must consist of 7 parts after the extension is removed from the name, and:

2. part 1 is the project code; an arbitrary set of letters (including diacritics, Ã, Â, etc) and numbers.
3. part 2 is the discipline that the file relates to.
4. part 3 is the project phase.
5. part 4 is a 4-digit document number in the format of xxxx (0001, 0002, etc...)
6. part 5 is the subject that the document relates to.
7. part 6 is the floor that the project relates to.
8. part 7 is the revision number; the format is RXX (R00, R01, etc...)
9. parts must be in said order.

Parts 2, 3, 5, and 6 must each be an abbreviation in a predefined set of values. Validating them is a simple matter of looking up if the abbreviate exists.

I wrote a single class for each part. For the sake of brevity I included only one class out of four. But assume all four are identical. The only difference is the constant of acceptable abbreviations.

class FileName
  attr_reader :name

  def self.valid?(name)
    new(name).valid?
  end

  # Valid file name
  # ABCD-ARQ-AP-0022-ACS-LOC-R00.jpeg

  def initialize(name)
    # Split individual parts into an array, ignoring .extension
    @name = name.split('.').first.split('-')
  end

  def valid?
    name.length == 7       &&
    project_code_valid?    &&
    discipline_valid?      &&
    phase_valid?           &&
    document_number_valid? &&
    subject_valid?         &&
    level_valid?           &&
    revision_valid?
  end

  def project_code
    @project_code ||= name[0]
  end

  def project_code_valid?
    project_code !~ /\P{Alnum}/ && project_code.length == 4
  end

  def discipline
    @discipline ||= name[1]
  end

  def discipline_valid?
    Discipline.value_valid?(discipline)
  end

  def phase
    @phase ||= name[2]
  end

  def phase_valid?
    Phase.value_valid?(phase)
  end

  def document_number
    @document_number ||= name[3]
  end

  def document_number_valid?
    document_number !~ /[^0-9]/ && document_number.length == 4
  end

  def subject
    @subject ||= name[4]
  end

  def subject_valid?
    Subject.value_valid?(subject)
  end

  def level
    @level ||= name[5]
  end

  def level_valid?
    Level.value_valid?(level)
  end

  def revision
    @revision ||= name[6].split('R')
  end

  def revision_valid?
    revision.length == 2 &&
    revision.first.empty? &&
    revision.last !~ /[^0-9]/ &&
    revision.last.length == 2
  end
end

class Discipline
  DISCIPLINES = {
    'Acessibilidade'                        => 'ACE',
    'Acústica'                              => 'ACU'
    # Snip...
  }

  def self.value_valid?(value)
    new(value).value_valid?
  end

  def initialize(value)
    @value = value
  end

  def value_valid?
    DISCIPLINES.values.include?(@value)
  end
end

class Phase
  # Snip...
end

class Subject
  # Snip...
end

class Level
  # Snip...
end

Usage

  # files_helper.rb
  def file_name_valid?(file)
    unless FileName.valid?(file.name)
      content_tag :span, 'Invalid File Name', class: 'label-danger'
    end
  end

Issues I have with the code

1. The aforementioned classes for parts 2, 3, 5, and 6 have a lot of duplication. As mentioned, they are identical except for the hash of acceptable values. Maybe inheritance is a good option?
2. The valid? method looks very long winded.
3. The classes are floating around in the file, raising the possibility of a name clash.
4. The FileName class should be instantiated once. It should be a singleton. I'm not sure how to implement singletons in Rails.
5. There's a lot of memoization (@level ||= name[6]) in the accessor methods I wrote for the different name parts.
6. Can the project_code_valid? and document_number_valid? methods be more compact? Can Regex be used to check string length instead of having a separate check?
7. All abbreviations (parts 2, 3, 5, and 6) should be uppercase; my regexes do not account for this.
8. Again in parts parts 2, 3, 5, and 6, I am instantiating the class each time. Should this be memoized?
9. The revision method seems too magical.
   1. It splits the string by R. A valid rev should produce an array of two elements: ["", "00"].
   2. The first element of this array has to be empty? because splitting by R removes it and returns an empty string.
   3. And the last element has to be a number made up of two digits. I'm not sure this will be readable in 5 months.
10. The same can be said about the readability of my accessor methods that retrieve the parts. E.g. @level ||= name[5]. There are important details hidden in name[5]. And that is the 5th part should represent a level. This does not seem obvious enough.

Answer 1

Issues you have with the code

1. The aforementioned classes for parts 2, 3, 5, and 6 have a lot of duplication. As mentioned, they are identical except for the hash of acceptable values. Maybe inheritance is a good option?

   You're right that they are repetitive, but the validators are are short and clear. I'd leave them alone.

2. The valid? method looks very long winded.

   It's not too bad. You are, after all, running seven different validators on seven parts of the string.

3. The classes are floating around in the file, raising the possibility of a name clash.

   Do the classes Discipline, Phase, Subject, and Level exist solely for the purpose of filename validation? If so, they might be overkill, but you could always put them inside a module as a namespace to avoid interfering with other code. If they are being used for other purposes by the rest of your application as well, then you shouldn't have to worry about clashing with your own code, right?

4. The FileName class should be instantiated once. It should be a singleton. Not sure how to implement singletons in Rails.

   I don't see why you would want to make FileName a singleton. Perhaps you mean to say that you only want to call class methods on FileName? (See #8 below.)

5. There's a lot of memoization (@level ||= name[6]) in in the accessor methods I wrote for the different name parts.

   Memoization for something as trivial as extracting an array element is absurd. You could just as easily write:

   def level
     name[6]
   end
   

   But why not assign @level in the constructor and declare an attr_reader :level instead? In fact, you just got confused yourself: the level is actually name[5]. Better to not bother with array indexes at all — see my solution below.

6. Maybe the project_code_valid? and document_number_valid? methods can be more compact? Can Regex be used to check string length instead of having a separate check?

   Yes. Get rid of the double negative (negated match against a regex that checks for bad characters) and just ask for exactly what you want to accept.

   def project_code_valid?
     project_code =~ /\A[A-Z0-9]{4}\z/
   end
   

7. All abbreviations (parts 2, 3, 5, and 6) should be uppercase; my regexes do not account for this.

   If you want uppercase, use an uppercase character class.

8. Again in parts parts 2, 3, 5, and 6, I am instantiating the class each time. This should be memoized?

   Memoization is not the solution. If you don't want to instantiate objects, use class methods instead.

   class Discipline
     DISCIPLINES = {
       'Accessibilidade'  => 'ACE',
       'Acústica'         => 'ACU',
     }
   
     def self.value_valid?(value)
       DISCIPLINES.values.include?(value)
     end
   end
   

   Similarly, if the only purpose of the FileName class is so that you can call FileName.valid?(…) and get a simple yes/no answer, then just use class methods instead of instance methods.

9. The revision method seems too magical.

   Indeed, I find it weird that it returns a two-element array whose first element is an empty string. What good is that to the caller? Wouldn't it be better to return just the two-digit string instead?

10. The same can be said about the readability of my accessor methods that retrieve the parts. E.g. @level ||= name[5]. There are important details hidden in name[5]. And that is the 5th part should represent a level. This does not seem obvious enough.

    I'm not sure what you mean by this question. If you're saying that LOC is an abbreviation that stands for something, then maybe filename.level should return a Level object instead of a string.

Proposed solution

class FileName
  attr_reader :name
  attr_reader :project_code, :discipline, :phase, :document_number, :subject, :level

  # Exceptional implementation below
  # attr_reader :revision

  def self.valid?(name)
    new(name).valid?
  end

  # Valid file name
  # ABCD-ARQ-AP-0022-ACS-LOC-R00.jpeg

  def initialize(name)
    @name = name

    # Split individual parts into an array, ignoring .extension.
    # If there are fewer than seven hyphen-delimited parts, then
    # @revision will be nil and validation will fail.  If there
    # are more than seven parts, then @revision will contain a
    # hyphen and validation will fail.
    parts = name.split('.', 2).first.split('-', 7)
    @project_code, @discipline, @phase, @document_number, @subject, @level, @revision = parts
  end

  def valid?
    # Testing revision first will detect if there are not exactly 7 parts earlier.
    revision_valid?        &&
    level_valid?           &&
    subject_valid?         &&
    document_number_valid? &&
    phase_valid?           &&
    discipline_valid?      &&
    project_code_valid?
  end

  # If revision is valid, return it with the leading 'R' stripped off.
  def revision
    revision_valid? ? @revision[1..-1] : @revision
  end

  def project_code_valid?
    project_code =~ /\A[\p{Lu}\d]{4}\z/
  end

  def discipline_valid?
    Discipline.value_valid?(discipline)
  end

  def phase_valid?
    Phase.value_valid?(phase)
  end

  def document_number_valid?
    document_number =~ /\A\d{4}\z/
  end

  def subject_valid?
    Subject.value_valid?(subject)
  end

  def level_valid?
    Level.value_valid?(level)
  end

  def revision_valid?
    @revision =~ /\AR\d{2}\z/
  end
end

Answer 2

I don't know if you really need to create classes for checking each of the substrings in the file name prefix. After all, there are only two types of checks that need to be made: against a list or matching a regex. Consider a simple, straighforward approach like this:

FNAME_SECTION = [
  {offset: 0, name: "Project code"   , regex: /^\p{Alnum}{4}$/ },
  {offset: 1, name: "Discipline"     , list: ['ACE', 'ARQ']    },
  {offset: 2, name: "Project phase"  , list: ['AP', 'BP']      },
  {offset: 3, name: "Document number", regex: /^\d{4}$/        }, 
  {offset: 4, name: "Subject"        , list: ['ACS', 'BCS']    },
  {offset: 5, name: "Level"          , list: ['LOC', 'KOV']    },
  {offset: 6, name: "Revision"       , regex: /^R\d{2}$/       }     
  ]

.

def fname_valid?(fname)
  @groups = fname.split('.').first.split('-')
  if @groups.size != FNAME_SECTION.size
    puts "Filename should have #{FNAME_SECTION.size} groups, but has #{@groups.size}"
    return nil
  end      

  err = []

  FNAME_SECTION.each_with_index do |h,i|
    str = @groups[h[:offset]] 
    if h.key?(:list)
      err << i unless h[:list].include?(str)
    elsif h.key?(:regex)
      err << i unless str =~ h[:regex]
    else
      err << i
    end 
  end

  if err.empty?
    puts "File name prefix is valid"
    return true
  end

  puts "File name prefix is invalid"
  err.each {|i| puts loc_msg(i)}
  return false
end

.

private

def loc_msg(i)
  "  Error in group offset #{FNAME_SECTION[i][:offset]} (#{FNAME_SECTION[i][:name]})"
end

.

fname_valid?('ABCD-ARQ-AP-0022-ACS-LOC-R00.jpeg')
  # File name prefix is valid
fname_valid?('ABC7-ACE-CP-002a-BCS-LOc-R000.jpeg')
  # File name prefix is invalid
  #   Error in group offset 2 (Project phase)
  #   Error in group offset 3 (Document number)
  #   Error in group offset 5 (Level)
  #   Error in group offset 6 (Revision)
fname_valid?('ABCD-ARQ-AP-0022-ACS-LOC.jpeg')
  # Filename prefix should have 7 groups, but has 6

The way I've displayed the error messages may not be what you want, but that would not be difficult to change. Note that, when a file name has an invalid format, I've listed all the reasons it is invalid.

When matching a substring against a regex, notice that the length of the substring is checked by including start/end anchors and avoiding the use of re+, re* and re?.

For validity checks that involve a list of possible values, I've made the list an array of the values from your hashes, as the keys did not appear to be used. If the keys are needed, those arrays could be replaced with hashes.

A potential problem with this approach is that it's not very flexible. If, for example, a validity check were changed to involve something other than matching a list or a regex, it might be difficult to alter the code to accommodate it.

I initially considered a different approach that offered greater flexibility. It retained the array of hashes, FNAME_SECTION, possibly changed somewhat, but also had a module that looked something like this:

module CustomValidityChecks
  def document_number_valid?
    ...
  end

  def revision_valid?
    ...
  end
end

This module contains the validity checks that could not be done from the information in FNAME_SECTION alone. The following is executed in the main class, when it is parsed:

@custom_validity_checks = CustomValidityChecks.instance_methods(false)

This saves all those methods in the class instance variable @custom_validity_checks. One could then use the earlier approach to make the validity checks that draw only on the information in FNAME_SECTION, and cycle through @custom_validity_checks to perform the others:

@custom_validity_checks.each { |m| send(m) }

Note that methods can be added to or deleted from the module (or renamed), with no need to alter any of the other code.

A variant of this approach would be create a subclass of the main class for each of these custom checks, and then use the hook Class#inherited to build the array @custom_validity_checks.