HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Outputting years with no repeated digits

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
repeatedoutputtingdigitswithyears

Problem

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:

  • there is an easier way to make an array from a string with no spaces



  • 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



  • 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.

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#144629, answer score: 2

Revisions (0)

No revisions yet.