HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Checking host status for clients on server

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
clientscheckingstatushostforserver

Problem

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 '' #{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

Solution

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

Context

StackExchange Code Review Q#18796, answer score: 7

Revisions (0)

No revisions yet.