Checking host status for clients on server

Question

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).

• Flambino: +1 - I think you touched on everything. Only thing I'd add is that 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.
• tokland: @Flambino: I also frowned upon all theses prefixed vhost_, you explained it very well, edited.
• SecurityGate: I know this is sort of a silly question, but how do you learn if you don't ask, right? :) Are using bash commands within ruby programs just a bad practice, or is there a performance hit involved also?
• tokland: @SecurityGate: someone said there are no stupid questions, only stupid answers :-) Yeah, it will be slower if you call external programs, no doubt, but I don't think there's a serious performance hit unless you create a lot of processes (and you know, Linux for example is very efficient on creating new processes). If it's a bad practice is mainly because it makes the script less portable and mixes unnecessarily another language and tools when Ruby is already a great language to do text processing.
• Flambino: @SecurityGate As tokland just wrote, it's certainly not "wrong" to invoke shell commands in Ruby. It's just that your code is doing so much shell invocation, that Ruby ends up being the unnecessary element in the mix. I.e. the "meat" of all three methods is shell commands anyway, so you could no doubt write it all as a plain bash script -- or you could use Ruby to a fuller extent, and gain some nice code (it's a lovely language). It's just that right now, your code is doing both and neither, so to speak.

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.