On 3 February 2013 11:13, Soichi Ishida <lists / ruby-forum.com> wrote:

> ruby 1.9.3p362 (2012-12-25 revision 38607) [x86_64-darwin12.2.1]
>
> I am trying to analyze a file and extract necessary data from it.
> First I need to count the total number of lines and the blank lines. the
> class and methods follow.
>
> class DataExtractor
>     attr_reader :file_name
>     def initialize(file)
>         @file_name = file
>     end
>
>     def total_lines
>         @lines = 0
>         f = File.open(@file_name, 'r')
>         f.read.each_line do |l|
>             @lines += 1
>         end
>         f.close
>         @lines
>     end
>
>     def total_blank_lines
>         regEx = /^[\s]*$\n/
>         @total_blank_lines = 0
>         f = File.open(@file_name, 'r')
>         f.read.each_line do | line |
>             if line =~ regEx
>                 @total_blank_lines += 1
>             end
>         end
>         f.close
>         @total_blank_lines
>     end
>     ....
>
> The problem is that each method has 'File.open...each_line do ' so the
> whole loop gets executed separately.
>
> Don't you think this is inefficient?  Is there better ways to develop
> methods so that the loop is needed only once?
>

Yep, sure do; and yep, sure is.  The fact that you're using @variables
inside your methods is already a hint at how to improve things -- have the
object remember them.  My first approach would be to do all the file
analysis in the constructor, sort of like this (using your code; it would
look a little different if I wrote it for myself):

    class DataExtractor
      attr_reader :file_name, :lines, :total_blank_lines
      def initialize(file)
        @file_name = file
        @lines = 0
        @total_blank_lines = 0
        regEx = /^[\s]*$\n/
        f = File.open(@file_name, 'r')
        f.read.each_line do | line |
          if line =~ regEx
            @total_blank_lines += 1
          end
          @lines += 1
        end
        f.close
      end
    end

My next approach would be to lazy-initialise the data, because that's just
a thing I like to do.  It means the lines aren't counted until they're
needed, in case that's an improvement. (Again, rough code, untested.)

    class DataExtractor
      attr_reader :file_name

      def initialize(file)
        @file_name = file
        @lines = -1
        @total_blank_lines = -1
      end

      def lazy_init_lines
        regEx = /^[\s]*$\n/
        f = File.open(@file_name, 'r')
        f.read.each_line do | line |
          if line =~ regEx
            @total_blank_lines += 1
          end
          @lines += 1
        end
        f.close
      end

      def lines
        lazy_init_lines if @lines < 0
        @lines
      end

      def total_blank_lines
        lazy_init_lines if @total_blank_lines < 0
        @total_blank_lines
      end
    end

In either case you perform a single open-read-close loop, and can access
the totals over and over again.  Various further optimisations and
improvements are available to be made at your discretion.

-- 
  Matthew Kerwin, B.Sc (CompSci) (Hons)
  http://matthew.kerwin.net.au/
  ABN: 59-013-727-651

  "You'll never find a programming language that frees
  you from the burden of clarifying your ideas." - xkcd