3
\$\begingroup\$

I'm working through a course now, and was tasked with the following:

Write a function, no_repeats(year_start, year_end), which takes a range of years and outputs those years which do not have any repeated digits.

You should probably write a helper function, no_repeat?(year) which returns true/false if a single year doesn't have a repeat.

I did not know about each_char yet, so I wrote the following:

def no_repeats(year_start, year_end)
  (year_start..year_end).to_a.keep_if {|year| no_repeat?(year)}
end

def no_repeat?(year)
  year_array = year.to_s.split(//)
  year_array == year_array.uniq
end

I'm wondering if:

  1. there is an easier way to make an array from a string with no spaces
  2. there is a way to convert each character in a number to an item in a single array without converting the number to a string first
  3. there is a better way to do this problem

This is the model solution for a beginner:

def no_repeats(year_start, year_end)
  no_repeats = []
  (year_start..year_end).each do |yr|
    no_repeats << yr if no_repeat?(yr)
  end

  no_repeats
end

def no_repeat?(year)
  chars_seen = []
  year.to_s.each_char do |char|
    return false if chars_seen.include?(char)
    chars_seen << char
  end

  return true
end

I'm also wondering about the advantages and disadvantages of my code compared to the model solution. I kind of get that shorter code is better, and readability matters. I don't know how to test code for time yet.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

The model solution they propose is very unidiomatic, your code is far better. I'd just made some minor changes:

  • Use String#chars instead of String#split(//).
  • Use Enumerable#select? instead of Array#keep_if to avoid modifying data in-place. It's better to work with immutable streams of data (with methods from Enumerable).

I'd write:

def no_repeats(year_start, year_end)
  (year_start..year_end).select { |year| no_repeat?(year) }
end

def no_repeat?(year)
  year_digits = year.to_s.chars
  year_digits == year_digits.uniq
end
\$\endgroup\$
2
  • \$\begingroup\$ thank you for the help. I see exactly why you changed the variable name from "array" to "digits" and why chars is a clearer method than split. I'm curious why you'd take select over keep_if. Please explain. \$\endgroup\$ Oct 19, 2016 at 11:33
  • \$\begingroup\$ Array#keep_if changes the original array in-place. In this particular code that's not a problem, but as a general rule, it's not a good idea, I guess you've read about Functional Programming. In Ruby you usually work with a functional (immutable) streams of data, using Enumerable methods. github.com/tokland/tokland/wiki/RubyFunctionalProgramming \$\endgroup\$
    – tokland
    Oct 19, 2016 at 11:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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