Skip to main content
added 51 characters in body
Source Link
200_success
  • 145.7k
  • 22
  • 191
  • 481

The POSIX grep command accepts the keyword as the first parameter, then the paths to search as subsequent parameters. That has the advantage of letting the user specify any number of paths to search.

You misspelled "argumetns". Also, I recommend writing "#{File::basename($0)}" rather than hardcoding "finder" as the program name.

It is customary to put all of the class / method definitions first, then all of the "main" code at the end. You've put the argument-handling code at the top, and the file_finder(expand_path(path), keyword) call at the bottom. Curiously, you test ARGV.length in three places — is it really that important?

file_finder

Avoid using Dir#chdir, as changing the current directory alters the global state of the process. One consequence of Dir#chdir is that you need to use absolute paths. For portability, use File::join(path, item) instead of path + "/" + item.

For efficiency, IGNORED_DIRS should be a Set.

def file_finder(path, keyword)
  Dir.foreach(path) do |item|
    next if IGNORED_DIRS.include?(item)

    item_path = File::join(path, item)
    if File.file?(item_path)
      search_in_file(item_path, keyword)
    else
      file_finder(item_path, keyword)
    end
  end
end

expand_path

Your expand_path incorrectly treats an absolute path as a relative path.

When expanding a path starting with ~, I don't see any reason to provide __FILE__ as the second argument to File.expand_path.

In case of an error, you should raise an exception instead of printing a message here. However, I don't see any reason why there should be an error in the first place.

If you avoid changing directories altogether, then you wouldn't need to generate absolute paths at all. Then you could just eliminate this function and call File::expand_path instead.

search_in_file

Opening a file without closing it causes a file descriptor leak.

flag is a poor variable name.

I'm not a fan of augmenting the String class, especially for code that is, strictly speaking, related more to terminal control than strings.

def search_in_file(path_to_file, keyword)
  seen = false
  File::open(path_to_file) do |f|
    f.each_with_index do |line, i|
      if line.include?(keyword)
        if not seen
          puts path_to_file.bold.blue
          seen = true
        end
        puts "#{i+1}:".bold.gray + " #{line}".sub(keyword, keyword.bg_red)
      end
    end
  end
  puts "" if seen
end

The POSIX grep command accepts the keyword as the first parameter, then the paths to search as subsequent parameters. That has the advantage of letting the user specify any number of paths to search.

You misspelled "argumetns". Also, I recommend writing "#{File::basename($0)}" rather than hardcoding "finder" as the program name.

It is customary to put all of the class / method definitions first, then all of the "main" code at the end. You've put the argument-handling code at the top, and the file_finder(expand_path(path), keyword) call at the bottom. Curiously, you test ARGV.length in three places — is it really that important?

file_finder

Avoid using Dir#chdir, as changing the current directory alters the global state of the process. One consequence of Dir#chdir is that you need to use absolute paths. For portability, use File::join(path, item) instead of path + "/" + item.

def file_finder(path, keyword)
  Dir.foreach(path) do |item|
    next if IGNORED_DIRS.include?(item)

    item_path = File::join(path, item)
    if File.file?(item_path)
      search_in_file(item_path, keyword)
    else
      file_finder(item_path, keyword)
    end
  end
end

expand_path

Your expand_path incorrectly treats an absolute path as a relative path.

When expanding a path starting with ~, I don't see any reason to provide __FILE__ as the second argument to File.expand_path.

In case of an error, you should raise an exception instead of printing a message here. However, I don't see any reason why there should be an error in the first place.

If you avoid changing directories altogether, then you wouldn't need to generate absolute paths at all. Then you could just eliminate this function and call File::expand_path instead.

search_in_file

Opening a file without closing it causes a file descriptor leak.

flag is a poor variable name.

I'm not a fan of augmenting the String class, especially for code that is, strictly speaking, related more to terminal control than strings.

def search_in_file(path_to_file, keyword)
  seen = false
  File::open(path_to_file) do |f|
    f.each_with_index do |line, i|
      if line.include?(keyword)
        if not seen
          puts path_to_file.bold.blue
          seen = true
        end
        puts "#{i+1}:".bold.gray + " #{line}".sub(keyword, keyword.bg_red)
      end
    end
  end
  puts "" if seen
end

The POSIX grep command accepts the keyword as the first parameter, then the paths to search as subsequent parameters. That has the advantage of letting the user specify any number of paths to search.

You misspelled "argumetns". Also, I recommend writing "#{File::basename($0)}" rather than hardcoding "finder" as the program name.

It is customary to put all of the class / method definitions first, then all of the "main" code at the end. You've put the argument-handling code at the top, and the file_finder(expand_path(path), keyword) call at the bottom. Curiously, you test ARGV.length in three places — is it really that important?

file_finder

Avoid using Dir#chdir, as changing the current directory alters the global state of the process. One consequence of Dir#chdir is that you need to use absolute paths. For portability, use File::join(path, item) instead of path + "/" + item.

For efficiency, IGNORED_DIRS should be a Set.

def file_finder(path, keyword)
  Dir.foreach(path) do |item|
    next if IGNORED_DIRS.include?(item)

    item_path = File::join(path, item)
    if File.file?(item_path)
      search_in_file(item_path, keyword)
    else
      file_finder(item_path, keyword)
    end
  end
end

expand_path

Your expand_path incorrectly treats an absolute path as a relative path.

When expanding a path starting with ~, I don't see any reason to provide __FILE__ as the second argument to File.expand_path.

In case of an error, you should raise an exception instead of printing a message here. However, I don't see any reason why there should be an error in the first place.

If you avoid changing directories altogether, then you wouldn't need to generate absolute paths at all. Then you could just eliminate this function and call File::expand_path instead.

search_in_file

Opening a file without closing it causes a file descriptor leak.

flag is a poor variable name.

I'm not a fan of augmenting the String class, especially for code that is, strictly speaking, related more to terminal control than strings.

def search_in_file(path_to_file, keyword)
  seen = false
  File::open(path_to_file) do |f|
    f.each_with_index do |line, i|
      if line.include?(keyword)
        if not seen
          puts path_to_file.bold.blue
          seen = true
        end
        puts "#{i+1}:".bold.gray + " #{line}".sub(keyword, keyword.bg_red)
      end
    end
  end
  puts "" if seen
end
Source Link
200_success
  • 145.7k
  • 22
  • 191
  • 481

The POSIX grep command accepts the keyword as the first parameter, then the paths to search as subsequent parameters. That has the advantage of letting the user specify any number of paths to search.

You misspelled "argumetns". Also, I recommend writing "#{File::basename($0)}" rather than hardcoding "finder" as the program name.

It is customary to put all of the class / method definitions first, then all of the "main" code at the end. You've put the argument-handling code at the top, and the file_finder(expand_path(path), keyword) call at the bottom. Curiously, you test ARGV.length in three places — is it really that important?

file_finder

Avoid using Dir#chdir, as changing the current directory alters the global state of the process. One consequence of Dir#chdir is that you need to use absolute paths. For portability, use File::join(path, item) instead of path + "/" + item.

def file_finder(path, keyword)
  Dir.foreach(path) do |item|
    next if IGNORED_DIRS.include?(item)

    item_path = File::join(path, item)
    if File.file?(item_path)
      search_in_file(item_path, keyword)
    else
      file_finder(item_path, keyword)
    end
  end
end

expand_path

Your expand_path incorrectly treats an absolute path as a relative path.

When expanding a path starting with ~, I don't see any reason to provide __FILE__ as the second argument to File.expand_path.

In case of an error, you should raise an exception instead of printing a message here. However, I don't see any reason why there should be an error in the first place.

If you avoid changing directories altogether, then you wouldn't need to generate absolute paths at all. Then you could just eliminate this function and call File::expand_path instead.

search_in_file

Opening a file without closing it causes a file descriptor leak.

flag is a poor variable name.

I'm not a fan of augmenting the String class, especially for code that is, strictly speaking, related more to terminal control than strings.

def search_in_file(path_to_file, keyword)
  seen = false
  File::open(path_to_file) do |f|
    f.each_with_index do |line, i|
      if line.include?(keyword)
        if not seen
          puts path_to_file.bold.blue
          seen = true
        end
        puts "#{i+1}:".bold.gray + " #{line}".sub(keyword, keyword.bg_red)
      end
    end
  end
  puts "" if seen
end