2
\$\begingroup\$

I wrote this Ruby Mod 11 (perhaps it's a bit generous to call it that) implementation to validate Brazilian CPF documents, which use a Mod 11 algorithm. CPF format is a 9 digit number followed by two checksums. It's often presented in this format 012.345.678-90. Feedback is welcome.

class CPF
  attr_reader :digits

  LENGTH = 11

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

  def self.mask(number)
    new(number).mask
  end

  def initialize(number)
    @digits = number.to_s.gsub(/\D/, '').each_char.map(&:to_i)
  end

  def valid?
    correct_length? && !black_listed? && checksums_match?
  end

  # The `black_listed?` method checks common number patterns.
  #
  # It tests to see if the provided CPF is the same digit repeated n times.
  #
  def black_listed?
    digits.join =~ /^12345678909|(\d)\1{10}$/
  end

  # A valid CPF must be 11 digits long.
  #
  def correct_length?
    digits.size == LENGTH
  end

  # A CPF is only valid if the first and second checksum digits are what they should be.
  #
  def checksums_match?
     first_checksum_matches? && second_checksum_matches?
  end

  # This returns the CPF is a human readable format.
  #
  #     CPF.mask("01234567890")
  #     => "012.345.678-90"
  #
  def mask
    [digits.first(9).each_slice(3).map(&:join).join("."), digits.last(2).join].join('-') if valid?
  end

private

  def first_checksum_matches?
    checksum_one == digits.at(9)
  end

  def second_checksum_matches?
    checksum_two == digits.at(10)
  end

  def checksum_one
    digit = sum_at(10)
    digit = calculate(digit)
    digit = 0 if digit > 9
    digit
  end

  def checksum_two
    digit = sum_at(11) + (2 * checksum_one)
    digit = calculate(digit)
    digit = 0 if digit > 9
    digit
  end

  def sum_at(position)
    digits.slice(0..8).collect.with_index { |n, i| n * (position - i) }.reduce(:+)
  end

  def calculate(digit)
    LENGTH - (digit % LENGTH)
  end
end

Tests

require 'test_helper'

class CpfTest < ActiveSupport::TestCase
  test 'black listed numbers are not' do
    black_list = %w(00000000000 11111111111 22222222222 33333333333 44444444444
                    55555555555 66666666666 77777777777 88888888888 99999999999)
    black_list.each do |number|
      assert_invalid number
    end
  end

  test 'nil is not a valid CPF' do
    assert_invalid nil
  end

  test 'blank is not a valid CPF' do
    assert_invalid ''
  end

  test 'valid CPF' do
    assert_valid '01234567890'
  end

  test 'masked valid CPF' do
    assert_valid '012.345.678-90'
  end

  test 'mask returns a masked CPF when CPF is valid' do
    assert_equal '012.345.678-90', CPF.mask('01234567890')
  end

  test 'mask returns nil when CPF is not valid' do
    assert_nil CPF.mask('0123456789')
  end

private

  def assert_invalid(number)
    refute CPF.valid?(number), "Expected #{number || 'nil'} to be an invalid CPF"
  end

  def assert_valid(number)
    assert CPF.valid?(number), "Expected #{number} to be a valid CPF"
  end
end
\$\endgroup\$
  • \$\begingroup\$ I was wondering why you've called the method that returns a string mask. I think the name is non-obvious, at least to me (might be obvious in your context, though). Why not simply use the default to_s instead? \$\endgroup\$ – DarkDust Aug 29 '14 at 17:37
  • \$\begingroup\$ @DarkDust good point. I called it mask because such numbers are usually stored without any formatting. For for display, however, after retrieval, one may want to mask the number. I guess that shouldn't stop me from using a to_s. \$\endgroup\$ – Mohamad Aug 29 '14 at 19:12
  • \$\begingroup\$ @DarkDust also, it's to allow me to use the class method CPF.mask("01234567890") #=> "012.345.678-90" \$\endgroup\$ – Mohamad Aug 29 '14 at 19:27
  • \$\begingroup\$ Although the updated code was added before the first answer, I've removed it since the first version has already been reviewed. Keeping the updated code may cause some confusion. \$\endgroup\$ – Jamal Aug 30 '14 at 0:51
5
\$\begingroup\$

Your code looks pretty good, but there is a no-no:

def checksum_one
  digit = sum_at(10)
  digit = calculate(digit)
  digit = 0 if digit > 9
  digit
end

Note thatdigit my have up to three different values in just four lines of code. This makes the code harder to follow. As a rule of thumb, use different variable names. That's how I'd write it:

def checksum_one
  digit = calculate(sum_at(10))
  digit > 9 ? 0 : digit
end
\$\endgroup\$
  • \$\begingroup\$ Thanks! As you might have noticed your previous code reviews have taught me to use more functional programming. Thanks for that. \$\endgroup\$ – Mohamad Aug 29 '14 at 19:17
  • \$\begingroup\$ You're welcome! Yeah, I noticed some functional bits in your code, they look nice :) \$\endgroup\$ – tokland Aug 29 '14 at 19:18
2
\$\begingroup\$

Tokland already gave a fine answer. I'll just offer a few more suggestions here and there.

  • The underscore in black_listed implies that it's two words in regular English. However, "blacklist" and "blacklisted" are in single words each.

  • The mask method could perhaps be more "direct" with a regular expression:

    def mask
      digits.join.sub(/(\d{3})(\d{3})(\d{3})(\d{2})/, '\1.\2.\3-\4') if valid?
    end
    

    Incidentally, I think simply overriding to_s would be better than having a method named "mask". It's not entirely clear what "masking a CPF" would mean.

  • I'd perhaps add some methods to pull the "payload" digits and the checksum digits, e.g.

    def checksums
      digits[-2, 2]
    end
    
    def payload
      digits[0, 9]
    end
    

    This could perhaps make the checksum-checking simpler, like so (note the use of Array#rotate)

    def calculate_checksum(digits)
      sum = digits.map.with_index { |digit, i| digit * (i+1) }.inject(&:+)
      sum % 11 % 10
    end
    
    def checksums_match?
      checksums == [ calculate_checksum(payload), calculate_checksum(payload.rotate) ]
    end
    

    This avoid having two very similar methods to check either checksum individually.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review. Great feedback. Code reviews always seem to make me feel perpetually bad at programming, but teach me a heck of a lot! \$\endgroup\$ – Mohamad Aug 30 '14 at 18:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.