Look-and-say sequence

Question

Here is my code in Ruby that shows the look-and-say sequence. It shows correct result, but my teacher said, that I could actually make it better. Can you tell me what the flaws are there in my code?

In the input I tell how many sequence elements I want to see. For example I type

5

and get:

1
11
21
1211
111221

class NumberSequence
 def self.start
 puts 'Enter the number of elements in sequence'
 count = gets.to_i - 1
 number = "1"
 times = 1
 puts number
 count.times do
 result = ""
 c = number[0]
 number = number[1..-1]+" "
 number.chars.each do |char|
 if(c!=char)
 result+= times.to_s+c
 times = 1
 c = char
 else
 times+=1
 end
 end
 number = result
 puts(result)
 end
end
end
NumberSequence.start

Answer 1

Let's start with the criticism: For the 'problems' I see in your actual code:

• There's no comment, comment your code, you'll be glad to have a help from yourself when you get to the code later after working on totally unrelated things
• Some indentation is wrong (But maybe it's when copy/pasting here), use a correct editor (like Atom or SublimeText, or any IDE with formatting capabilities to help you). I fixed it in the extract above.
• you have useless lines of code like number = result just before the last puts which have no purpose.
• You have a single method, this could have be out of a class with the same effect

Now for the constructive I can tell on those bullet points:

Reading your code you have roughly 4 parts:

1. Getting the user input

   puts 'Enter the number of elements in sequence'
   count = gets.to_i - 1
   

2. Initializing variables

   number = "1"
   times = 1
   

3. Iterating and building the result

   count.times do
    result = ""
    c = number[0]
    number = number[1..-1]+" "
    number.chars.each do |char|
    if(c!=char)
    result+= times.to_s+c
    times = 1
    c = char
    else
    times+=1
    end
    end
   

4. Printing the result

   puts(result)
   

In an object oriented language, the interest of a class if to separate responsibilities and do small part of code which can be easily tested.

Here is how I would implement the challenge, I'm not a ruby expert, so there's maybe still improvement I don't see, I'll give a comparison with yours under it.

# encoding: utf-8
class NumberSequence
 # Initialize the instance variables
 def initialize
 @count = 0
 end
 # Ask the user for how many lines and store in instance variable
 def get_input
 puts 'Enter the number of elements in sequence'
 @count = gets.to_i - 1
 end
 # Build the next line based on the line given in parameter
 # @param [String] line
 def build_next_line(line)
 newline = ''
 occurrence = 0
 last_char = line[0]
 line.each_char do |char| # Iterate over the line characters
 if char != last_char # If the current char is different from previous one
 newline = "#{newline}#{occurrence}#{last_char}" # Append to the newline string
 occurrence = 0 # and reset the counter
 last_char = char # update the last char seen
 end
 occurrence += 1
 end
 # the last char is not added to newline within the loop, let's add it in the return
 "#{newline}#{occurrence}#{last_char}"
 end
 def print_output
 # Build an array of lines to avoid growing the object in loop
 out = Array.new(@count,String)
 # Fill up the first line
 out[0] = '1'
 (1..@count).each do |n| # starting on second line
 out[n] = build_next_line(out[n-1]) # Build current line
 end
 puts out.join("\n") # print the result
 end
end
# Get an instance of the class
seq = NumberSequence.new
# Get the input from user
seq.get_input
# Print the resulting text output
seq.print_output

I did chose to stay with only one instance variable, the number of desired lines '@count', all the rest could stay in the methods, one arguable point is the ouput text which, to be even cleaner, could go in an instance variable and use a method to just print it.

I isolated the the user supplied entry, I didn't go further on the road, but ideally this method should ensure it is effectively an integer which has been retrieved.

Then a method to make the output, as we are nesting loops to compute each line the inner code can be isolated to allow it to be tested separately (i.e line by line).

We now have 3 methods which can get tested separately:

• get_input
• build_next_line
• print_output

Now on the syntax itself, I do prefer to use string interpolation "#{variable}text#{other var}" than + as I find it more readable and less confusing with arithmetic (and sometimes you may end up raising exceptions when trying to concatenate two objects like this).

For the line loop, I iterate on a range instead of count, and last syntax I try to stick with is using single quotes for strings without interpolation and double quotes only when there's interpolation.

• SuperBiasedMan: Looks good! Thorough explanations of your points. Welcome to the site. :)

Answer 2

Interface design

You've lumped all of the functionality into one function. At the least, you should have a function that is responsible for sequence-generating, and another for the prompting and printing.

To me, this problem cries out to be treated as a enumerator. With an enumerator, you would automatically benefit from all of the members of the Enumerable mixin, especially #take, which takes the first n elements.

Since this is an infinite sequence, one tricky bit is to ensure that you define a lazy enumerator so that it doesn't try to generate the entire infinite sequence.

Implementation

The implementation would be a lot simpler if you took advantage of Enumerable#chunk.

Suggested solution

module LookAndSaySequence
 # An enumerator that generates the sequence "1", "11", "21", "1211", ...
 # (or with some other initial string of digits)
 def self.strings(init='1')
 Enumerator.new do |y|
 s = init
 loop do
 y << s
 s = s.chars
 .chunk { |digit| digit }
 .map { |digit, list| list.length.to_s + digit }
 .join
 end
 end.lazy
 end
end
print 'Enter the number of elements in sequence: '
LookAndSaySequence.strings.take(gets.to_i).each { |s| puts s }

... and a bonus variant LookAndSaySequence.numbers that produces Fixnums instead of Strings:

module LookAndSaySequence
 # An enumerator that generates the sequence 1, 11, 21, 1211, ...
 # (or with some other initial number)
 def self.numbers(init=1)
 strings(init.to_s).map { |s| s.to_i }
 end
end

• tokland: I had an almost identical code prepared (only relevant difference: [1] instead of "1"). +1

Answer 3

Seems like you are putting the whole script inside the class. The idea of using a class is to have a group of "cohesive" methods that work on instance variables, representing an object and its "valid" interactions with the outside world. Each method should do one specific thing, either accessing directly the instance variables and/or composing other methods. Make private any method that is pointless or dangerous to invoke from outside the class.

I also would leave out of the class anything related with I/O (reading input from user or printing output). Think of the "LookAndSay" class as a module that could be reused without changes in a different i/o context, for example in a REST service.