Request for suggestions of coding practice improvements

[ignusb]

Hello,
I've written a basic class to help check on the status of various services, to be used in hopefully an extensible and easily modified manner..
I'm just a ruby/scripting/coding newbie, using tutorials online to learn from and working it all out as I go. I was hoping to get feedback and suggestions from users here for what I can do to improve further and whether I'm doing anything a strange or less efficient way than I could.

The idea of this is that using the add_checker(...) function, I can define an ip, port, data to send, data expected to receive back and a graceful closing block in case a protocol requires exit commands.

I then use the run() function to run through all the checkers I've added.

The other thing I'm not sure on is how to add exception handling for the case where the socket doesn't connect correctly.
Even if someone could point me in the right direction instead of giving me the code for that, it would be appreciated.

require 'socket'

$sleeptime = 60
$debug = true
$failthreshold = 5

def debuglog(text)
  if $debug == true
    puts text
  end
end

class Checker
  def initialize()
    @servicestruct = Struct.new(:server,    \
                                :port,      \
                                :protocol,  \
                                :senddata,  \
                                :exp_recv,  \
                                :closing,   \
                                :action,    \
                                :actionarg, \
                                :failcount)
    @services = []
  end
  
  private
    
    def email(address, servicename)
      #when fail_count = $failthreshold, if action = email, send message to specified email as in actionarg
      #stub function:
      if 1==1 then
        return 0
      else
        return 1
      end
      end
    end #def email
    
    def restart(server, port)
      #when fail_count = $failthreshold, if action = restart, attempt restart of service on server actionarg
      #stub function:
      if 1==1 then
        return 0
      else
        return 1
      end
      end
    end #def restart
  
    def check(service) #Pass nil for send or closing data if not applicable
      if service.protocol.casecmp("TCP") == 0 then
        sock = TCPSocket::new(service.server, service.port) ################### TODO: Handle error where cannot connect
        if service.senddata != nil then
          sock.print(service.senddata)
        end
        sleep(1)
        if sock.read =~ /#{service.exp_recv}.*/s then
          service.failcount = 0
          debuglog("#{service.server}:#{service.port} Passed test")
        else
          service.failcount += 1
          debuglog("#{service.server}:#{service.port} Failed test")
        end
        if service.closing != nil then
          sock.print(service.closing)
        end
      end # if protocol == TCP

      if service.protocol.casecmp("UDP") == 0 then
        sock = UDPSocket::new
        sock.bind(nil, service.port)
        sock.send(service.senddata, 0, service.server, service.port)
        if sock.recvfrom(service.exp_recv.length) =~ /#{service.exp_recv}.*/ then
          service.failcount = 0
        else
          service.failcount += 1
        end
        if service.closing != nil then
          sock.send(service.closing, 0, service.server, service.port)
        end
      end # if protocol == UDP

      if service.failcount == $failthreshold then
        timestamp = Time.now.to_s
        servicemsg =" #{timestamp}  Service failed for #{$sleeptime} seconds -  #{service.server}:#{service.port}\n"
        puts servicemsg.chomp
        
        if service.action.casecmp("email") == 0 then
          if email(actionarg, servicemsg) == 1 then
            puts "Service status email sent to #{actionarg}"
          else
            puts "Service status email failed to send to #{actionarg}"
          end
        end
        
        if service.action.casecmp("restart") == 0 then
          if restart(service.ip, service.port) == 1 then
            puts "Restart succeeded on #{service.ip}:#{service.port}"
          else
            puts "Restart failed on #{service.ip}:#{service.port}"
          end
        end
        
      end #if failcount == 5
    end #def check
      
  public
    def add_checker(server, port, protocol, senddata, exp_recv, closing, action, actionarg)
      #populate @servicestruct
      tmpstruct = @servicestruct.new(server, port, protocol, senddata, exp_recv, closing, action, actionarg, 0)
      
      #If UDP, must have data to send.
      if protocol.casecmp("UDP") && senddata == nil then
        puts "UDP service checker failed for #{server} on #{port}. Must give data to send."
        return -1
      end
              
      #add to @services
      @services.push(tmpstruct)
    end
    
    def run
      if @services.size == nil || @services.size == 0 then
        puts "Must add service checkers before you can run Checker"
        return -1
      end
      for service in @services      
        check(service)
      end # for service in @services
    end # def run
end # class Checker

s = Checker.new
  
s.add_checker("192.168.164.7", 80, "TCP", \
              "GET /wmtestdata/ HTTP/1.0\r\n\r\n", \
              "HTTP/1.1 200 OK", nil, "stdout", nil)

while 1
  s.run()
  if $debug == 1
    sleep(1)
  else
    sleep($sleeptime)
  end
end

[tiger86]

Thats some scary code you have there. You've written it in basically all globals, which is very dangerous if you plan to deploy this application onto the internet. You need to use instances, methods, classes, and try to keep the use of globals to a bare minimum or not at all. A global in ruby is

$

an instance in ruby is i.e

@cool

. I hope that helps.