How Much Stratification do I really want in Ruby?

Question

I was referred over to codereview, from stack overflow, since I recently found out that my programming style wasn't the best. I taught myself Ruby, and unfortunately I don't have anyone to compare my work to, so I've been doing my best with what I have. I have a background in C, so I kept writing procedural programs which apparently isn't in best practices.

I run a small server, and to check whether clients are being hosted on my server, I wrote this. I'm new to object oriented programming, how stratified should my programs really be? Do I want to have every possible variable and piece in their own function? Also, how am I doing in general? Any recommendations?

class Domain_check
  def vhost_grab
    return full_domain_path = `ls /etc/httpd/conf.d/vhost_* | grep -v 000_defaults.conf`.chomp.split(' ')
  end

  def vhost_stripper
    prefix =  Domain_check.new.vhost_grab
    vhost_stripped = []
    prefix.each_index do |x|
      vhost_stripped[x] = `echo '#{prefix[x]}' | awk -F'vhost_' '{print $2}' | awk -F'.conf' '{print $1}'`
    end
    return vhost_stripped
  end

  def vhost_display
    puts "\n%s %40s %43s" %["Domain name", "IP Address Listed", "IP Address Currently in Use"]
    final_vhost = vhost_stripper
    final_vhost.each_index { |x|
      padding = 50
      print final_vhost[x].strip
      padding = padding.to_i - final_vhost[x].length
      print "%0#{padding}s" %[`grep '<VirtualHost .*:80>' #{Domain_check.new.vhost_grab[x]} | awk -F'<VirtualHost' '{print $2}'|awk -F':' '{print $1}'`.strip.to_s]
      puts "%40s" %[`dig #{final_vhost[x].strip} +short`]
    }
  end
end

d1 = Domain_check.new
d1.vhost_display

Answer 1

Some observations:

• Regarding the "stratification". It's hard to give practical advice, this is something that comes from experience and personal taste. Some basics to start with: a) write classes/modules with low coupling to achieve real modularization, b) write fairly short methods than do only one thing. c) Keep model-view separations. d) Don't use global variables.

• class Domain_check: that's against Ruby practices, a class or module names are named CamelCase: class DomainCheck.

• Don't write an explicit return on the last line of a method/block, it's non idiomatic.

• Don't assign a variable on this last expression, why would be it different from the method name itself?

• Don't call to external commands (ls, grep, awk, ...), Ruby is more than able to perform those tasks, check the standard library.

• About this: prefix = DomainCheck.new.vhost_grab. You are already in the instance, just prefix = vhost_grab.

• You are using a class just nominaly, you don't use any of its facilities. You can convert all these methods to classmethods if you store nothing in the instances.

• Init empty + iterate with each + push is an anti-pattern in Ruby (and in any language with decent functional capabilities, for that matter). Use Enumerable#map. Also, you use each_index to iterate in a C fashion, use each (or better, map, select, inject, as required, read the Enumerable documentation from start to finish).

• Having only a C background your code has a serious problem, it's very, very imperative. Functional programming allows far better abstraction and clarity, check this wiki page I maintain.

• If every method is prefixed with vhost_ there's no point in prefixing anything. Besides, if all the methods start with vhost_, the class should probably be called VirtualHostChecker instead (@Flambino).

Answer 2

I would highly recommend picking up a copy of "Clean Code" by Robert C. Martin. All of the example code in the book is in Java, but as a Ruby developer with absolutely no Java background I was still able to get a lot out of the book.

It won't teach you Ruby specific conventions like when to use CamelCaseNames (classes and modules) vs underscored_names (methods and variables) vs ALL_UPPERCASE (constants). It does, however, do an excellent job of explaining how to write good, readable, object oriented code that is highly applicable to any OO language.

For instance, it would probably have taught you to..

• rename the Domain_check class to something more appropriate, such as VirtualHostPrinter, since that is, in fact, what this class does.
• not prefix your method names with vhost_ (Chapter 2, "Meaningful Names", "Avoid Encodings/Member Prefixes" section)
• use method and variable names to describe precisely what they do
• break complex methods down into more/smaller methods that do only one thing each (see Single Responsibility Principal)
• etc.

Below is your code after I applied some refactoring. I would have done more with the awks and greps to refactor those into pure ruby, but my awk is rusty and I wasn't sure exactly what you were trying to achieve.

class VirtualHostPrinter
  class << self

    def engage
      print_formatted_header
      domain_paths.each do |domain_path|
        print_virtual_host_for(domain_path)
      end
    end

    #######
    private
    #######

    def print_formatted_header
      puts "\n%s %40s %43s" %["Domain name", "IP Address Listed", "IP Address Currently in Use"]
    end

    def domain_paths
      `ls /etc/httpd/conf.d/vhost_* | grep -v 00_defaults.conf`.chomp.split(' ')
    end

    def print_virtual_host_for(domain_path)
      print strip(domain_path)
      print "%0#{padding_for(domain_path)}s" %[`grep '<VirtualHost .*:80>' #{domain_path} | awk -F'<VirtualHost' '{print $2}'|awk -F':' '{print $1}'`.strip.to_s]
      puts "%40s" %[`dig #{final_vhost[x].strip} +short`]
    end

    def strip(domain_path)
      `echo '#{domain_path}' | awk -F'vhost_' '{print $2}' | awk -F'.conf' '{print $1}'`
    end

    def padding_for(domain_path)
      50 - strip(domain_path).length
    end
  end
end

VirtualHostPrinter.engage

Note, that I haven't tested this at all to see if it actually works, and outside the full context of the rest of the program, it's hard to say if any of this actually makes any sense.

Also note that I didn't just rewrite your code from scratch. I made a copy of your code and then applied lots and lots of very small incremental changes. Much of my intermediate code got completely removed as later refactorings made it obsolete. There's lots more refactoring that could be done to make this even more clean and readable, but I think this is a step in the right direction.