Return to Answer

• 145.5k
• 22
• 190
• 478

• 145.5k
• 22
• 190
• 478

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[A-Z0-9]{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[0-9]{4}\z/
 end
 def subject_valid?
 Subject.value_valid?(subject)
 end
 def level_valid?
 Level.value_valid?(level)
 end
 def revision_valid?
 @revision =~ /\AR[0-9]{2}\z/
 end
end