5
\$\begingroup\$

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
\$\endgroup\$
0

3 Answers 3

3
\$\begingroup\$

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.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Looks good! Thorough explanations of your points. Welcome to the site. :) \$\endgroup\$ Commented Nov 12, 2015 at 17:09
4
\$\begingroup\$

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
\$\endgroup\$
1
  • \$\begingroup\$ I had an almost identical code prepared (only relevant difference: [1] instead of "1"). +1 \$\endgroup\$
    – tokland
    Commented Nov 12, 2015 at 20:19
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

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