2
\$\begingroup\$

I have this class that needs to do three things:

  • Determine if a string is 8 characters or longer
  • Determine if a string has a number in it
  • Determine if a string has a capital letter in it

NB: I don't want to use regex. Is there a better way of doing this?

class ValidateString
  def initialize(string)
    @string = string

    if valid 
      puts @string 
    end
  end

  def valid
    if has_number? and has_upper? and has_length?
      return @string
    end
  end

  def has_number?
    @string.count("0-9") > 0
  end

  def has_upper?
    @string =~ /[A-Z]/
  end

  def has_length?
    @string.length >= 8
  end
end
\$\endgroup\$
  • \$\begingroup\$ Why not use regex? It's sounds like a valid solution for this \$\endgroup\$ – Flambino Apr 3 '17 at 1:23
  • 2
    \$\begingroup\$ (And you are already using regex in has_upper? by the way) \$\endgroup\$ – Flambino Apr 3 '17 at 1:30
  • 1
    \$\begingroup\$ Are you checking for password strength? Obligatory XKCD \$\endgroup\$ – 200_success Apr 3 '17 at 1:43
2
\$\begingroup\$
  • ValidateString is strange name for a class. StringValidator would be more apt.

  • Don't make your constructor print anything. Your class exists to validate a string, not print things to stdout. Leave that to some other piece of code.

  • valid should be named valid? and return a boolean. And you can omit the return keyword.

  • Add attr_reader :string, and replace your @string with just string

  • 8 is a magic number, and should be replaced with constant (e.g. MIN_STRING_LENGTH)

  • Presumably, this exists to check for simple passwords. However, you're not checking that the string contains both upper- and lowercase letters. An all-uppercase string with a number would pass validation. For passwords, you'd want to enforce that it's mixed casing.

I'd just write a method:

def valid_string?(string)
  return false unless string.length >= 8
  return false unless string =~ /[0-9]/
  return false unless string =~ /[[:upper:]]/ # allows non-Latin letters
  return false unless string =~ /[[:lower:]]/ # ditto
  true
end

Of course, you'd be better off using something like zxcvbn to check password entropy. Your rules allow for the good ol' Password1 string just fine.

\$\endgroup\$
  • \$\begingroup\$ "8 is a magic number, and should be replaced with constant (e.g. MIN_STRING_LENGTH)" Could you elaborate on this? Everything else makes perfect sense to me. I'm still learning the appropriate time to use constants. Why exactly should the 8 alone not be used? \$\endgroup\$ – user135185 Apr 3 '17 at 13:22
  • 1
    \$\begingroup\$ @fight_dragons Because 8 by itself isn't very explanatory, whereas MIN_STRING_LENGTH makes it clear what you're doing. Granted, this isn't a terrible case, since the context makes it clear enough ("ok, string has to be at least 8 chars long, got it"), but still. If in doubt: Use a constant. Any arbitrary number you use in your code should be "labelled" by making it a constant. It keeps the code much more maintainable. \$\endgroup\$ – Flambino Apr 3 '17 at 13:57
  • \$\begingroup\$ return false unless string =~ /[[:lower:]] is not asked in the specification, your function incorrectly rejects 12345689X that should be accepted. \$\endgroup\$ – Caridorc Apr 4 '17 at 17:40
  • 1
    \$\begingroup\$ @Caridorc Read my answer again; the last point in particular. I'm well aware I'm making an assumption about the code's purpose. \$\endgroup\$ – Flambino Apr 4 '17 at 17:51
  • 1
    \$\begingroup\$ @Flambino yup I had missed that, fair point \$\endgroup\$ – Caridorc Apr 4 '17 at 17:52
0
\$\begingroup\$

@Flambino 's answer can be simplified by using a simple && (and) instead of return false unless (I also removed the extra condition):

def valid_string?(string)
  string.length >= 8       && \     
  string =~ /[0-9]/        && \
  string =~ /[[:upper:]]/ 
end
\$\endgroup\$

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.